[tree-shaking bug] sideEffects doesn't seem to be working correctly

[pd4d10]

Describe the bug

When importing the package lowlight, some parts of the code in files listed at sideEffects are removed incorrectly:

https://github.com/wooorm/lowlight/blob/0803fc3416cbdf59f204da243313e3315eb34617/lib/common.js#L1-L36

Reproduce steps

1. Open the reproduction link below
2. run npm run build
3. Check if the JS output has these highlight.js languages code bundled in

Reproduction

https://stackblitz.com/edit/vitejs-vite-zyomf8?file=main.js

System Info

It has been reproduced at Stackblitz. So ignore this part.

Used Package Manager

npm

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[pd4d10]

Additional information

These lines of code may be related:

vite/packages/vite/src/node/packages.ts

Lines 97 to 105 in 7186857

	const { sideEffects } = data
	let hasSideEffects: (id: string) => boolean
	if (typeof sideEffects === 'boolean') {
	hasSideEffects = () => sideEffects
	} else if (Array.isArray(sideEffects)) {
	hasSideEffects = createFilter(sideEffects, null, { resolve: pkgDir })
	} else {
	hasSideEffects = () => true
	}

But it seems to have done the right thing.

[sapphi-red]

I think this behavior is correct.

1. lowlight has index.js in main field.
2. index.js has export {lowlight} from './lib/common.js'.
3. lib/common.js includes imports from highlight.js and uses them.
4. Also sideEffects field declares that lib/common.js has side-effect.

If you intend not to include highlight.js languages automatically I think you should import lowlight/lib/core.js instead of lowlight as it is written here.
https://github.com/wooorm/lowlight/tree/main#what-is-this

[pd4d10]

If you intend not to include highlight.js languages automatically

On the contrary, we intend to include them. The problem is they are removed unexpectedly

[sapphi-red]

Sorry I misread it.

[sapphi-red]

I have found that changing either of below will make it not occur.

1. change import {lowlight} from 'lowlight' to import {lowlight} from 'lowlight/lib/common'

2. change export {lowlight} from './lib/common' to

   import {lowlight} from './lib/common'
   export {lowlight}

So I think it is related to re-exporting.

[sapphi-red]

I was able to reproduct only with rollup.
Maybe it is a bug in rollup?
https://stackblitz.com/edit/node-lr98tg

[pd4d10]

Thanks for investigating it!

I'm not sure if Rollup respects the sideEffects field. According to the code snippet in my previous comment here #7635 (comment), it seems that Vite has some specific logics?

[sapphi-red]

@rollup/plugin-node-resolve has a simillar logic with vite.
https://github.com/rollup/plugins/blob/8fadc64c679643569239509041a24a9516baf340/packages/node-resolve/src/util.js#L142-L151
https://github.com/rollup/plugins/blob/23226cdd67118cff29beb9cbd3a436091d34a7c2/packages/node-resolve/src/index.js#L223

I feel rollup is incorrectly treating moduleSideEffects when there is a re-export.

[lukastaegert]

Fix at rollup/rollup#4867

[sapphi-red]

Thanks!

Closing as rollup 3.17.2 that includes the fix was released.