Skip to content

docs: add information about "sideEffects"#10691

Merged
benmccann merged 17 commits intosveltejs:mainfrom
rossrobino:patch-1
Jun 19, 2024
Merged

docs: add information about "sideEffects"#10691
benmccann merged 17 commits intosveltejs:mainfrom
rossrobino:patch-1

Conversation

@rossrobino
Copy link
Contributor

@rossrobino rossrobino commented Sep 7, 2023

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • [] Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

sideEffects

I recently discovered that the CSS from every component in my library (you can test by importing from 3.0.6 vs 3.0.7) was being included in the build when only one component was being used. By adding "sideEffects": false to package.json it was resolved. I figured this would be good to add to the docs to minimize bundle sizes for component libraries.

@changeset-bot
Copy link

changeset-bot bot commented Sep 7, 2023

⚠️ No Changeset found

Latest commit: 5bc1a85

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@benmccann
Copy link
Member

This looks really good. The main thing I noted while looking at this is that most Svelte users use Vite and the Vite docs say nothing about sideEffects. I took a quick look though and do see some references to it in the Vite code, so I'd imagine there's some level of support. It may be worth at least filing an issue with Vite noting the absence of this feature from their docs

@rossrobino
Copy link
Contributor Author

Thanks! Sounds good, I've opened an issue, we can see what they say.

@rossrobino
Copy link
Contributor Author

I do see mention of side effects in rollup's docs, but I can't find it in Vite's either.

@benmccann benmccann added the documentation Improvements or additions to documentation label Sep 7, 2023
rossrobino and others added 3 commits September 8, 2023 09:09
Co-authored-by: Willow (GHOST) <ghostdevbusiness@gmail.com>
Co-authored-by: Willow (GHOST) <ghostdevbusiness@gmail.com>
Co-authored-by: Willow (GHOST) <ghostdevbusiness@gmail.com>
@dominikg
Copy link
Contributor

dominikg commented Sep 9, 2023

see this issue in vite vitejs/vite#4389 (via #10633)

A blanket recommendation for "sideEffects": false can break libraries that actually have side effects and rely on them to work (in my opinion svelte libraries generally should not, and explicitly listing the effectful files is better, but that isn't mentioned at all).

Before we change the documentation i'd like to see a repository demonstrating that and how it works.

Other options to explore:

  1. Tagging the css module returned from vite-plugin-svelte load with moduleSideEffect: false https://rollupjs.org/plugin-development/#load (although i'm not sure if/how vite handles these for css)

  2. Not using a barrel index.js file to list all components, but an exports map that maps deep imports to root, so they are less cumbersome to use

"exports": {
  "svelte": {
    "import": {
      "./Foo.svelte": "./src/components/Foo.svelte" 
    }
  }
}

(I did experiment with globs in exports map a while ago but back then it was buggy for *.svelte)

@benmccann
Copy link
Member

in my opinion svelte libraries generally should not, and explicitly listing the effectful files is better, but that isn't mentioned at all

I agree they shouldn't, so think it would be nice to make that the recommendation. I think this PR both mentions the dangers of getting it wrong and shows how to explicitly list the files with side effects where it says "Make sure that "sideEffects" is correctly set. If a file with side effects is incorrectly marked as having no side effects, it can result in broken functionality. If your package has files with side effects, you can specify them in an array:"

Not using a barrel index.js file to list all components, but an exports map that maps deep imports to root, so they are less cumbersome to use

We could mention that would also address the issue in this section. I don't think we should go as far as recommending users always do it as it seems to be a case-by-case basis as to which might be the better design for a library

@dominikg
Copy link
Contributor

did some investigation here: sveltejs/vite-plugin-svelte#760

there are several ways in vite/rollup to say some module does not have a side effect, but currently the only one working seems to be the package.json field that's mentioned here. So unless there is a change that allows us to declare the virtual css modules from vite-plugin-svelte as free, the suggestion to add "sideEffects": ["your side effect files here"] to package.json of libraries is probably the best one.

If you are using barrel files inside your own application, you can also add this to the package.json in your app and it'll work.

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@dummdidumm
Copy link
Member

Judging from the discussion in #10691 (comment) it seems that bundlers work in weird ways (and may work differently across bundlers?) depending on the value of sideEffects. Until we got to the bottom of this I'll put this in draft mode (the text itself is well written though!)

@dummdidumm dummdidumm marked this pull request as draft December 12, 2023 16:16
@bertybot
Copy link
Contributor

bertybot commented Feb 2, 2024

Just throwing my hat into the ring here that this should at least be better advertised due to how focused Sveltekit is on performance. Right now I am maintaining a pretty mature Sveltekit app that imports a core Svelte library everywhere. Implementing this simple trick just gained me like 10 points on my lighthouse scores.

@benmccann
Copy link
Member

benmccann commented Jun 19, 2024

I'm marking this as ready for further review now that the issue I filed with Vite has been fixed and out for several releases.

I did a test on kit.svelte.dev:

current
du -ch .svelte-kit/output/client/_app/immutable/{entry,chunks}/.js | grep total
112K total
du -ch .svelte-kit/output/client/_app/immutable/assets/
.css | grep total
188K total

with "sideEffects": ["**/*.css"] in site-kit
du -ch .svelte-kit/output/client/_app/immutable/{entry,chunks}/.js | grep total
104K total
du -ch .svelte-kit/output/client/_app/immutable/assets/
.css | grep total
172K total

with "sideEffects": false in site-kit
du -ch .svelte-kit/output/client/_app/immutable/{entry,chunks}/.js | grep total
104K total
du -ch .svelte-kit/output/client/_app/immutable/assets/
.css | grep total
172K total

@benmccann benmccann marked this pull request as ready for review June 19, 2024 15:57
@benmccann
Copy link
Member

thank you!! it was a long journey on this one, but I'm glad we got it figured out. I'll follow up with a PR to update the default in create-svelte to follow the best practice discussed here

@benmccann benmccann merged commit bf36142 into sveltejs:main Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants