-
Notifications
You must be signed in to change notification settings - Fork 795
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
fix(compiler): support async globalScripts functions #5158
Conversation
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/mock-doc/serialize-node.ts | 36 |
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/compiler/config/test/validate-paths.spec.ts | 16 |
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 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2345 | 366 |
TS2322 | 362 |
TS18048 | 224 |
TS18047 | 99 |
TS2722 | 37 |
TS2532 | 26 |
TS2531 | 23 |
TS2454 | 14 |
TS2790 | 11 |
TS2352 | 10 |
TS2769 | 8 |
TS2538 | 8 |
TS2344 | 6 |
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 |
7a5e48c
to
c1431f0
Compare
c1431f0
to
5f0a992
Compare
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.
lgtm!
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.
Two additional asks:
- Can we update the PR + commit message to use semantic commits so this gets registered properly by the changelog
- Can we add a warning that this is a blocking call in https://stenciljs.com/docs/config#globalscript? In the repro case of the original issue they use 5 seconds as the delay, which although arbitrary, is quite a long time and made me concerned as first this wasn't working 😅
fixes #3392 STENCIL-467 PR feedback
5f0a992
to
cb5bea1
Compare
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/7833762155/artifacts/1231044941 If your browser saves files to
|
Thanks for the review, I addressed the comments.
Done 👍
Raised a PR with a proposal: stenciljs/site#1350 |
Side note (since I’m the original reporter of #3392). A use-case for supporting You can argue that it’s possible to structure the app to display a loader while information like this is fetched, but you can also do that by adding the loader to the In summary, you can probably work around this, but it’s really nice to have this supported by the compiler. |
Thanks for the feedback @mikaelkaron 🙏 |
What is the current behavior?
Scripts defined in the
globalScript
Stencil configuration won't delay the execution of the initialization process. However this is what the Stencil docs suggestfixes #3392
STENCIL-467
What is the new behavior?
With this patch Stencil will start recognize an asynchronous function and will wait for the async promise to resolve before continuing the initialisation.
Does this introduce a breaking change?
This could be a surprise for people that do async operations in these scripts but where not concerned about having their components being rendered before.
Testing
I added an e2e test but you can verify this patch by:
globalScript
that does async stuff (see test example)Other information
While looking for a fix for this issue I started to doubt the usefulness of this feature. I probably don't have enough context for how global scripts in Stencil are used but I believe there is are barely any reasons why you would stale the rendering of your application like that.
As an alternative I suggest to update our documentation to not promise async behavior. I assume this won't be a suprise for anyone since it didn't work before.