Skip to content
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: only inject entry scripts as <script> #46

Merged
merged 6 commits into from
Mar 24, 2023
Merged

fix: only inject entry scripts as <script> #46

merged 6 commits into from
Mar 24, 2023

Conversation

danielroe
Copy link
Member

Currently we are adding <script> tags with dynamic entries, e.g. they export an entry. The only effect of this should be to run any side-effects. I think we should omit these and just add them to the link preloads. They will be imported from the entry-file instead.

@danielroe danielroe added the bug Something isn't working label Feb 28, 2023
@danielroe danielroe requested a review from pi0 February 28, 2023 13:00
@danielroe danielroe self-assigned this Feb 28, 2023
@pi0
Copy link
Member

pi0 commented Feb 28, 2023

I think this comes from the webpack requirement of self-registering chunks. Can you please try if webpack (dynamic chunks) works fine with this change?

Also what are benefits of not doing this? Less HTML size?

@danielroe
Copy link
Member Author

Yes, that does make sense. I'll confirm, but think we would need to ensure we don't set isDynamicEntry when normalizing webpack manifests if so.

The issue actually manifested in that sometimes vite chunks depend on the entry already having been run (see nuxt/nuxt#18642 for example). Although that one was fixed, it feels better to evaluate the chunks in the order they expect to run.

@pi0
Copy link
Member

pi0 commented Feb 28, 2023

Thanks for explaining. Executing in order makes sense for vite indeed (webpack chunks in contrary self-register and evaluate in order by wp manifest)

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2023

Codecov Report

Merging #46 (48c7a29) into main (7b72ba2) will increase coverage by 0.04%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   95.23%   95.28%   +0.04%     
==========================================
  Files           6        6              
  Lines         525      530       +5     
  Branches      114      115       +1     
==========================================
+ Hits          500      505       +5     
  Misses         25       25              
Impacted Files Coverage Δ
src/runtime.ts 95.78% <100.00%> (+0.04%) ⬆️
src/types.ts 92.00% <100.00%> (+0.33%) ⬆️
src/webpack.ts 92.50% <100.00%> (+0.06%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/types.ts Outdated
@@ -6,6 +6,7 @@ export interface ResourceMeta {
assets?: string[]
isEntry?: boolean
isDynamicEntry?: boolean
selfRegister?: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt to rename to something like sideEffects: true (because on any side-effect, order matters).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants