-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
feat: async script module support, close #3163 #4864
Conversation
Looks great, appreciate the work on this @patak-js! |
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.
Awesome. Thanks for putting this together.
Thanks for the review. About mixed So we should at least issue a warning in this case, or directly error out the build. A hard error could be better if later we want to support the mixed case with a different strategy because that won't be a breaking change. If you have some input about this let me know. I'll bring this PR for approval to our next meeting with the team. |
a21d608
Added the warning that we discussed in the last meeting, and merged and corrected the PR to work properly after #4555 |
everyScriptIsAsync &&= isAsync | ||
someScriptsAreAsync ||= isAsync | ||
someScriptsAreDefer ||= !isAsync |
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.
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.
Is this a good or a bad reaction? 😅
We may need to refactor this info as an array and then at the end just use every/some over it, if thus is not clear
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.
It is a good reaction 🤣 It was just like a 😲
I also approved the PR 😸
Description
See #3163 for details
Additional context
This PR assumes that every script module is bundled together (even if several chunks may be generated, as with the main index and vendor chunks). Every script module needs to be marked as
async
for the output script tag to be async. Mixedasync
anddefer
would end up with adefer
tag as it is currently the default.An alternative would be to break the bundling in async and defer chunks that could be loaded separately, but this would involve a bigger refactoring that may not be justified. If we end up doing this, it could be introduced as a separate PR in the future.
This PR may have conflicts with #4555, but should be easy to resolve no matter which one we merge first. @andylizi you may want to review this PR to double check.
@DylanPiercey @ryansolid could you check that this PR would enable the use of
async
in Solid and Marko as you expected?Note: I'll send updated docs later to the PR
What is the purpose of this pull request?