Skip to content

fix: ignore sideEffects for scripts imported from html#12786

Merged
patak-cat merged 2 commits intomainfrom
fix/ignore-side-effects-for-scripts-imported-from-html
Apr 7, 2023
Merged

fix: ignore sideEffects for scripts imported from html#12786
patak-cat merged 2 commits intomainfrom
fix/ignore-side-effects-for-scripts-imported-from-html

Conversation

@patak-cat
Copy link
Member

@patak-cat patak-cat commented Apr 7, 2023

Fixes #10735

Description

It seems that adding moduleSideEffects: false to scripts imported from .HTML files pushes rollup to tree shake everything. Because "sideEffects": false is set in the package.json of the app, we end up doing this for every file.

This fix works for this reproduction: #10735 (comment)

I don't know if the fix should be more generic though. We could do this as a stop-gap if no one has a better idea here.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@bolt-new-by-stackblitz
Copy link

bolt-new-by-stackblitz bot commented Apr 7, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-cat patak-cat added p4-important Violate documented behavior or significantly improves performance (priority) feat: build labels Apr 7, 2023
@patak-cat patak-cat mentioned this pull request Apr 7, 2023
Closed
7 tasks
bluwy
bluwy reviewed Apr 7, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Makes sense to me, but would be great to have tests 😬

@patak-cat
Copy link
Member Author

patak-cat commented Apr 7, 2023

Done, thanks for the push ❤️

@patak-cat patak-cat requested a review from bluwy April 7, 2023 17:26
bluwy
bluwy approved these changes Apr 7, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Nice!

@patak-cat patak-cat enabled auto-merge (squash) April 7, 2023 17:34
@patak-cat patak-cat merged commit f09551f into main Apr 7, 2023
@patak-cat patak-cat deleted the fix/ignore-side-effects-for-scripts-imported-from-html branch April 7, 2023 17:36
@fregante
Copy link

fregante commented Apr 7, 2023

Let me know when this appears in a beta so I can add it to my PR.

thanks for working on this!

@patak-cat
Copy link
Member Author

patak-cat commented Apr 9, 2023

@fregante released in vite@4.3.0-beta.4

@fregante
Copy link

fregante commented Apr 9, 2023

Success! 38KB

❯ nr demo:build
vite v4.3.0-beta.4 building for production...
✓ 10 modules transformed.
3:36:08 PM [vite-plugin-svelte] dom compile done.
package             	files	  time	   avg
github-url-detection	    1	94.5ms	94.5ms
dist/index.html                  0.85 kB │ gzip:  0.49 kB
dist/assets/index-7b93ea5e.css   0.63 kB │ gzip:  0.34 kB
dist/assets/index-8dab0bc5.js   38.73 kB │ gzip: 10.58 kB
✓ built in 430ms

@sapphi-red sapphi-red mentioned this pull request Sep 19, 2024
Closed
3 tasks
@sapphi-red sapphi-red mentioned this pull request Oct 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bluwy bluwy bluwy approved these changes

Assignees

No one assigned

Labels

feat: build p4-important Violate documented behavior or significantly improves performance (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

v3.2.0 missing output

3 participants

@patak-cat @fregante @bluwy