feat: async script module support, close #3163 #4864
Conversation
|
Looks great, appreciate the work on this @patak-js! |
|
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. |
Shinigami92
added
the
p2-nice-to-have
patak-dev
dismissed stale reviews from Shinigami92, antfu, and ryansolid
via
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.
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.
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
asyncfor the output script tag to be async. Mixedasyncanddeferwould end up with adefertag 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
asyncin Solid and Marko as you expected?Note: I'll send updated docs later to the PR
What is the purpose of this pull request?