-
Notifications
You must be signed in to change notification settings - Fork 815
refactor(utils): move output-target related utils to @utils #4225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { isOutputTargetDocs } from '../compiler/output-targets/output-utils'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when I was fiddling with build stuff I noticed this relative import from import { isOutputTargetDocs } from '@stencil/core' but I thought that wasn't too great for a few reasons, mainly that it changes the public API of |
||
import { isOutputTargetDocs } from '@utils'; | ||
|
||
import type { ValidatedConfig } from '../declarations'; | ||
import type { CoreCompiler } from './load-compiler'; | ||
import { startupCompilerLog } from './logs'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,3 @@ | ||
import { isBoolean, isString } from '@utils'; | ||
import { isAbsolute, join, resolve } from 'path'; | ||
|
||
import type * as d from '../../../declarations'; | ||
import { | ||
COPY, | ||
DIST_COLLECTION, | ||
|
@@ -10,8 +6,13 @@ import { | |
DIST_LAZY_LOADER, | ||
DIST_TYPES, | ||
getComponentsDtsTypesFilePath, | ||
isBoolean, | ||
isOutputTargetDist, | ||
} from '../../output-targets/output-utils'; | ||
isString, | ||
} from '@utils'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we combine this with the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, thought I captured all of those! |
||
import { isAbsolute, join, resolve } from 'path'; | ||
|
||
import type * as d from '../../../declarations'; | ||
import { getAbsolutePath } from '../config-utils'; | ||
import { validateCopy } from '../validate-copy'; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
internal-platform-hydrate
bundle depends on functions which use the output-target utils which, in turn, importspath
. However, none of the functions which it uses actually use functioned exported bypath
, so when bundling it doesn't actually import anything frompath
but if you don't set this option Rollup will leave an unqualified import like so:if you say
moduleSideEffects: false
you're basically saying 'dont assume that imported a module has a desired side effect' which allows an import that doesn't result in any symbols being pulled into the importing module to be eliminated.Another way to say this is that without this change the treeshaking validation for
test.dist
doesn't pass for this bundle.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we capture this info in a comment for future us?