fix(build): address issue with dynamic import and vite #5399
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
| Path | Error Count |
|---|---|
| src/dev-server/index.ts | 37 |
| src/dev-server/server-process.ts | 32 |
| src/compiler/prerender/prerender-main.ts | 22 |
| src/testing/puppeteer/puppeteer-element.ts | 22 |
| src/runtime/client-hydrate.ts | 20 |
| src/screenshot/connector-base.ts | 19 |
| src/runtime/vdom/vdom-render.ts | 17 |
| src/dev-server/request-handler.ts | 15 |
| src/compiler/prerender/prerender-optimize.ts | 14 |
| src/compiler/sys/stencil-sys.ts | 14 |
| src/compiler/transpile/transpile-module.ts | 14 |
| src/sys/node/node-sys.ts | 14 |
| src/compiler/prerender/prerender-queue.ts | 13 |
| src/compiler/sys/in-memory-fs.ts | 13 |
| src/runtime/connected-callback.ts | 13 |
| src/runtime/set-value.ts | 13 |
| src/compiler/output-targets/output-www.ts | 12 |
| src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
| src/compiler/transformers/transform-utils.ts | 12 |
| src/mock-doc/test/attribute.spec.ts | 12 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2322 | 361 |
| TS2345 | 350 |
| TS18048 | 201 |
| TS18047 | 91 |
| TS2722 | 37 |
| TS2532 | 24 |
| TS2531 | 22 |
| TS2454 | 14 |
| TS2790 | 11 |
| TS2352 | 10 |
| TS2769 | 8 |
| TS2538 | 8 |
| TS2416 | 6 |
| TS2493 | 3 |
| TS18046 | 2 |
| TS2684 | 1 |
| TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
| File | Line | Identifier |
|---|---|---|
| src/runtime/bootstrap-lazy.ts | 21 | setNonce |
| src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
| src/testing/testing-utils.ts | 198 | withSilentWarn |
| src/utils/index.ts | 145 | CUSTOM |
| src/utils/index.ts | 269 | normalize |
| src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
| src/compiler/app-core/app-data.ts | 25 | BUILD |
| src/compiler/app-core/app-data.ts | 115 | Env |
| src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
| src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
| src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
| src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
alicewriteswrongs
force-pushed
the
ap/fix-vite-issue
branch
from
b1a7006 to
f78b645
Compare
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8020920200/artifacts/1269983155 If your browser saves files to |
alicewriteswrongs
force-pushed
the
ap/fix-vite-issue
branch
from
f78b645 to
4c0a927
Compare
christian-bromann
approved these changes
Feb 22, 2024
rwaskiewicz
reviewed
Feb 23, 2024
There was a problem hiding this comment.
I agree with @christian-bromann's comments here, nothing blocking on my end.
rwaskiewicz
approved these changes
Feb 23, 2024
alicewriteswrongs
force-pushed
the
ap/fix-vite-issue
branch
from
4c0a927 to
81a3abf
Compare
When we added a script for building the modules in `internal/` with Esbuild in #5276 we needed to make a change to the function that Stencil uses at runtime to lazy-load components (in `src/client/client-load-module.ts`). Prior to #5276 we had a dynamic import statement which looked like so: ```ts import( `./${bundleId}.entry.js${BUILD.hotModuleReplacement && hmrVersionId ? '?s-hmr=' + hmrVersionId : ''}` ) ``` This constructs a filepath to the module for a given Stencil component, accounting for HMR versioning, and then imports the module. All well and good, but unfortunately this dynamic import does not play well with Esbuild. As described [here](https://esbuild.github.io/api/#non-analyzable-imports) when Esbuild is in 'bundle' mode and it encounters an `import()` _and_ the imported path or identifier looks "analyzable" it will attempt to resolve the corresponding file and incorporate it into the bundle. This is not always what you want! In particular, in our situation the leading `"./"` in the template literal we had in `client-load-module.ts` caused Esbuild to consider the `import()` an "analyzable" import and it then tried to resolve and bundle the import instead of just leaving the dynamic import in the code (as Rollup does in this case). This created an issue because at _compile time_ (i.e. when Stencil itself is built) this import does not resolve to anything, so Esbuild would essentially transform that line into an empty import. This caused runtime issues because the side-effect of the dynamic import was no longer happening, so the modules containing Stencil component classes and so on were not longer being loaded in. To get this working for #5276 we pulled out the `"./"` string as a separate variable, changing the template literal so it looks something like this: ```ts const MODULE_IMPORT_PREFIX = './'; import( `${MODULE_IMPORT_PREFIX}${bundleId}.entry.js${BUILD.hotModuleReplacement && hmrVersionId ? '?s-hmr=' + hmrVersionId : ''}` ) ``` This causes Esbuild to conclude that the import is "non-analyzable", which addresses the issue and causes both Rollup and Esbuild to emit equivalent code for this snippet, where both retain the dynamic import, allowing for the runtime module resolution that we want here. _However_, this broke the ability to use Stencil with Vite, which will complain about non-analyzable imports if it sees a dynamic import which does _not_ begin with `"./"`. See #5389 for details. So essentially we have a situation where the behavior of Rollup, Esbuild, and Vite is incompatible. The solution is to figure out a way for both the Esbuild and Rollup builds to emit code in this case which retains the dynamic import _and_ retains the leading `"./"` in the template literal. This is accomplished by retaining the `${MODULE_IMPORT_PREFIX}` in the template literal, so that Esbuild does not attempt to analyze and bundle the import, and adding plugins to both the Rollup and Esbuild bundles to transform the emitted code before it is written to disk. fixes #5389 STENCIL-1181
alicewriteswrongs
force-pushed
the
ap/fix-vite-issue
branch
from
81a3abf to
e6568ac
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we added a script for building the modules in
internal/with Esbuild in #5276 we needed to make a change to the function that Stencil uses at runtime to lazy-load components (insrc/client/client-load-module.ts). Prior to #5276 we had a dynamic import statement which looked like so:This constructs a filepath to the module for a given Stencil component, accounting for HMR versioning, and then imports the module. All well and good, but unfortunately this dynamic import does not play well with Esbuild. As described
here when Esbuild is in 'bundle' mode and it encounters an
import()and the imported path or identifier looks "analyzable" it will attempt to resolve the corresponding file and incorporate it into the bundle.This is not always what you want! In particular, in our situation the leading
"./"in the template literal we had inclient-load-module.tscaused Esbuild to consider theimport()an "analyzable" import and it then tried to resolve and bundle the import instead of just leaving the dynamic import in the code (as Rollup does in this case).This created an issue because at compile time (i.e. when Stencil itself is built) this import does not resolve to anything, so Esbuild would essentially transform that line into an empty import. This caused runtime issues because the side-effect of the dynamic import was no longer happening, so the modules containing Stencil component classes and so on were not longer being loaded in.
To get this working for #5276 we pulled out the
"./"string as a separate variable, changing the template literal so it looks something like this:This causes Esbuild to conclude that the import is "non-analyzable", which addresses the issue and causes both Rollup and Esbuild to emit equivalent code for this snippet, where both retain the dynamic import, allowing for the runtime module resolution that we want here.
However, this broke the ability to use Stencil with Vite, which will complain about non-analyzable imports if it sees a dynamic import which does not begin with
"./". See #5389 for details.So essentially we have a situation where the behavior of Rollup, Esbuild, and Vite is incompatible. The solution is to figure out a way for both the Esbuild and Rollup builds to emit code in this case which retains the dynamic import and retains the leading
"./"in the template literal.This is accomplished by retaining the
${MODULE_IMPORT_PREFIX}in the template literal, so that Esbuild does not attempt to analyze and bundle the import, and adding plugins to both the Rollup and Esbuild bundles to transform the emitted code before it is written to disk.fixes #5389
STENCIL-1181
What is the current behavior?
GitHub Issue Number: N/A
What is the new behavior?
Documentation
Does this introduce a breaking change?
Testing
To test this the reproduction case linked in #5389 should be tried with both the ESbuild and Rollup based builds. It should work fine with either one.
The reproduction is here: https://github.com/gregorypratt/stencil-issue
After cloning locally, build and pack with Rollup and test to see that the issue doesn't repro anymore, and then do the same with Esbuild.
Other information