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

Global namespace pollution is still an issue #16443

Open
7 tasks done
matths opened this issue Apr 17, 2024 · 3 comments
Open
7 tasks done

Global namespace pollution is still an issue #16443

matths opened this issue Apr 17, 2024 · 3 comments
Labels
feat: library mode has workaround p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@matths
Copy link

matths commented Apr 17, 2024

Describe the bug

We are bundling Svelte components as custom elements using Vite.
We automized this step for a whole bunch of components and every now and then, there might be a component, whose bundle.js leads to conflicts with existing JS code on website we use the components. Worst case so far was a conflict with jQuery, because Vite bundling created a $ var in the global namespace.

There was a workaround PR for this more then a year ago, which does not work with minify: true.
#15489

I describe the problem and my findings in this discussion: #15489

Usually esbuild wraps everything inside an IIFE to have a scope other than the global scope so minified variables does not conflict. Also esbuild is adding those three to four variables, lets call them helpers.

Vite then again does not take advantage of this IIFE, but instead explicitly set the format option to undefined as stated in this comment:

// passing `var Lib = (() => {})()` to esbuild with format = "iife"
// will turn it to `(() => { var Lib = (() => {})() })()`,
// so we remove the format config to tell esbuild not doing this
//
// although esbuild doesn't change format, there is still possibility
// that `{ treeShaking: true }` removes a top-level no-side-effect variable
// like: `var Lib = 1`, which becomes `` after esbuild transforming,
// but thankfully rollup does not do this optimization now
iife: undefined,

I think it does so, because it has kind of a multi stage bundling mechanic and the first one already puts an IIFE around the code.

So the old workaround tried to move the variables inside the existing IIFE, but this does not work, when code was minified.

Reproduction

https://github.com/matths/global-namespace-pollution

Steps to reproduce

run npm install followed my npm run vite.
have a look at vite/bundle.js and vite/bundle.min.js and see that there are variables outside the IIFE which might conflict with global variables from other JS packages on the same website.

System Info

System:
    OS: macOS 14.4.1
    CPU: (12) arm64 Apple M2 Pro
    Memory: 2.11 GB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.6.1 - /opt/homebrew/bin/node
    npm: 10.2.4 - /opt/homebrew/bin/npm
    bun: 1.0.3 - ~/.bun/bin/bun
  Browsers:
    Chrome: 123.0.6312.124
    Edge: 123.0.2420.97
    Safari: 17.4.1
  npmPackages:
    vite: ^5.2.9 => 5.2.9

Used Package Manager

npm

Logs

No response

Validations

@sapphi-red
Copy link
Member

This seems to only happen when there's no exports. So a workaround is to add a dummy export export const foo = ''.

@sapphi-red sapphi-red added has workaround p2-edge-case Bug, but has workaround or limited in scope (priority) feat: library mode labels Apr 18, 2024
@matths
Copy link
Author

matths commented Apr 18, 2024

So can we conclude that compiling Svelte component as custom elements is using library mode but has no export but instead just defines a web component. And that's why there's no function wrapper around the code?

I tried that workaround, and indeed it can work as a workaround. I can add dummy exports to my entry points, of course. So, thank you, @sapphi-red.

But where to go from here? Do you suggest to add this workaround to Vite or to think about a more general solution that works without that dummy export?

@sapphi-red
Copy link
Member

I guess we can change the regex depending whether chunk.exports.length === 0 is true.

async renderChunk(code, chunk, opts) {

My hypothesis is that when it is true there is no (const|var)\s+\S+\s*=\s* part.

Adding a dummy export automatically will make the library declare a global variable (the name can be configured by output.name) so it's not ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: library mode has workaround p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

No branches or pull requests

2 participants