fix: respect "sideEffects: true" in package.json

[aleclarson]

Description

We can assume a package has side effects in this case. This is basically a way to force Rollup to always include the package when imported.

// Assume foo.js has side effects, skip static analysis.
// Assume any other modules have no side effects.
"sideEffects": ["./foo.js"],
// Assume every module in this package has side effects.
"sideEffects": true,

Additional context

So when moduleSideEffects is set to true (what Vite currently uses), Rollup will statically analyze the AST to determine if tree-shaking can occur, but in reality, we never want tree-shaking to occur for modules declared in pkg.sideEffects. To avoid tree-shaking (and potentially costly static analysis), moduleSideEffects must be set to no-treeshake.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[yyx990803]

There's a type error that needs to be fixed.

[Shinigami92]

I tried to find the issue, but the code looks right to me 🤔
Also moduleSideEffects is defined by rollup. I don't get any errors in my VSCode, only be the tests. So could it be that there is an issue with the tests? Maybe an outdated TS version or TS bug?!

[bluwy]

We'd likely need hasSideEffects = () => !!sideEffects && 'no-treeshake' since sideEffects is a string, we'd need to convert it to boolean explicitly.

[Shinigami92]

We'd likely need hasSideEffects = () => !!sideEffects && 'no-treeshake' since sideEffects is a string, we'd need to convert it to boolean explicitly.

But it is explicitly checked that it is typeof sideEffects === 'boolean' 🤔
If you hover over the code, it also says it can only be boolean
That let me think it is somekind of a bug

[bluwy]

Ah didn't notice that. Looks like you're right it is a bug, that's pretty strange. I'm not getting the warning in vscode though.

[aleclarson]

Seems to be a @rollup/plugin-typescript issue. I can't reproduce with tsc

[Shinigami92]

@aleclarson #6019 was merged, could you make a rebase of this PR? 🙂

[aleclarson]

Ready to merge ✅

[Niputi]

still failing on types

[aleclarson]

still failing on types

looks like Jest is using wrong tsconfig

[Shinigami92]

looks like Jest is using wrong tsconfig

Do we already have a PR to fix it? Could you link it here? Then we can observe the progress of that fix.

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	❌ failure
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt-framework	✅ success
previewjs	✅ success
qwik	❌ failure
rakkas	✅ success
sveltekit	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	❌ failure
vite-plugin-react-swc	✅ success
vite-plugin-svelte	⏹️ cancelled
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	❌ failure
vitest	✅ success
windicss	✅ success

[patak-dev]

There are no longer blockers now. I think we can merge this one.

[sapphi-red]

LGTM

But maybe we should create a PR to plugin-node-resolve so that we don't misalign with rollup's behavior.
https://github.com/rollup/plugins/blob/bc5cf91a042b88e14a1da9ba25f501d92591ca8a/packages/node-resolve/src/util.js#L143-L164

[patak-dev]

@sapphi-red I thought that we were choosing a different trade-off here, avoiding the AST analysis in exchange for possible bigger bundles because we aren't letting rollup tree-shake modules in packages with side effects (as we assume that most of the module will be needed in that case anyways). I think for rollup, the best is to still try to do its best to create smaller bundlers.

But thinking about the introduction of a misalignment with rollup, maybe this change isn't worth it. @aleclarson could you explain a bit more why you said:

we never want tree-shaking to occur for modules declared in pkg.sideEffects

There could be modules with side effects that export functions that could be tree-shaken, no?

From https://rollupjs.org/plugin-development/#load

If false is returned for moduleSideEffects and no other module imports anything from this module, then this module will not be included in the bundle even if the module would have side effects. If true is returned, Rollup will use its default algorithm to include all statements in the module that have side effects (such as modifying a global or exported variable). If "no-treeshake" is returned, treeshaking will be turned off for this module and it will also be included in one of the generated chunks even if it is empty. If null is returned or the flag is omitted, then moduleSideEffects will be determined by the first resolveId hook that resolved this module, the treeshake.moduleSideEffects option, or eventually default to true. The transform hook can override this.

[patak-dev]

@bluwy maybe you could help us with the question above?

[bluwy]

Looking at it now, I'm not sure if this is correct too. rollup/rollup#3663 is when rollup supported no-treeshake with the usecase. This PR would result in larger bundle size if the library is side-effectful, but you're only using one API.

[patak-dev]

@aleclarson please re-open the PR if we're missing something here. From the discussion above, I think we should close it for now.