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

Vite library build is polluting global namespace #11641

Closed
7 tasks done
adebicki opened this issue Jan 9, 2023 · 6 comments · Fixed by #11882
Closed
7 tasks done

Vite library build is polluting global namespace #11641

adebicki opened this issue Jan 9, 2023 · 6 comments · Fixed by #11882
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@adebicki
Copy link

adebicki commented Jan 9, 2023

Describe the bug

I am building a library. I have js file with class, which defines some fields. It is external npm package file, but the same occurs for simple code like this:

export class X {
 someField;
}

After build, iife and umd output files in dist folder are polluted with auxiliary global variables.

var __defProp = Object.defineProperty;
var __defNormalProp = (obj, key, value) => key in obj ? __defProp(obj, key, { enumerable: true, configurable: true, writable: true, value }) : obj[key] = value;
var __publicField = (obj, key, value) => {
  __defNormalProp(obj, typeof key !== "symbol" ? key + "" : key, value);
  return value;
};
(function(global, factory) {
...

After field declaration is removed, output files are as expected.

export class X {
  // someField;
}
(function(global, factory) {
...

Expected behavior: output files should not pollute global namespace

Reproduction

https://stackblitz.com/edit/vitejs-vite-vjtijq

Steps to reproduce

Run npm run build

System Info

(from stackblitz)

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 7.17.0 - /usr/local/bin/npm
  npmPackages:
    vite: ^4.0.4 => 4.0.4

Used Package Manager

npm

Logs

No response

Validations

@sapphi-red sapphi-red added regression The issue only appears after a new release p4-important Violate documented behavior or significantly improves performance (priority) and removed pending triage labels Jan 10, 2023
@cesutherland
Copy link

I encountered this or a related issue using the cjs build format — dependencies from node_modules polluted the global namespace.

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) and removed p4-important Violate documented behavior or significantly improves performance (priority) regression The issue only appears after a new release labels Jan 27, 2023
@sapphi-red
Copy link
Member

This happens only when esbuild.minifyWhitespace: false or build.minify: false is set.
Also this happens with v2.9.15 and v3.2.5.

@adebicki
Copy link
Author

adebicki commented Jan 30, 2023

Setting build.minify: true seems to resolve the problem in original reproduction example.

Problem still exists though.

When using @iabtcf/cmpapi npm library in file /node_modules/@iabtcf/cmpapi/lib/mjs/command/CommandMap.js we got class definition using bracket notation for field names. This seem to cause the problem again in umd build.

import { PingCommand } from './PingCommand.js';
import { GetTCDataCommand } from './GetTCDataCommand.js';
import { GetInAppTCDataCommand } from './GetInAppTCDataCommand.js';
import { GetVendorListCommand } from './GetVendorListCommand.js';
import { AddEventListenerCommand } from './AddEventListenerCommand.js';
import { RemoveEventListenerCommand } from './RemoveEventListenerCommand.js';
import { TCFCommand } from './TCFCommand.js';
export class CommandMap {
    static [TCFCommand.PING] = PingCommand;
    static [TCFCommand.GET_TC_DATA] = GetTCDataCommand;
    static [TCFCommand.GET_IN_APP_TC_DATA] = GetInAppTCDataCommand;
    static [TCFCommand.GET_VENDOR_LIST] = GetVendorListCommand;
    static [TCFCommand.ADD_EVENT_LISTENER] = AddEventListenerCommand;
    static [TCFCommand.REMOVE_EVENT_LISTENER] = RemoveEventListenerCommand;
}

Narrowing it down:

const foo = 'bar';

export class X {
  [foo];
}

Reproduction:
https://stackblitz.com/edit/vitejs-vite-bug-proof?file=lib%2Findex.js

@sun0day
Copy link
Member

sun0day commented Jan 31, 2023

This happens only when esbuild.minifyWhitespace: false or build.minify: false is set. Also this happens with v2.9.15 and v3.2.5.

Would it be OK to resolve this bug by moving extra global code into iife scope via magic-string?

var __defProp = Object.defineProperty;
var __defNormalProp = (obj, key, value) => key in obj ? __defProp(obj, key, { enumerable: true, configurable: true, writable: true, value }) : obj[key] = value;
var __publicField = (obj, key, value) => {
  __defNormalProp(obj, typeof key !== "symbol" ? key + "" : key, value);
  return value;
};
(function(global, factory) {
  typeof exports === "object" && typeof module !== "undefined" ? factory(exports) : typeof define === "function" && define.amd ? define(["exports"], factory) : (global = typeof globalThis !== "undefined" ? globalThis : global || self, factory(global.foo = {}));
})(this, function(exports2) {
  var _a;
  "use strict";
  // ==========> move `var __defProp....` here
  const foo = "bar";
  class X {
    constructor() {
      __publicField(this, _a);
    }
  }
  _a = foo;
  exports2.X = X;
  Object.defineProperty(exports2, Symbol.toStringTag, { value: "Module" });
});

@sapphi-red
Copy link
Member

@sun0day Yes, we are doing like that. I guess the regex is failing to catch these cases.

// #7188, esbuild adds helpers out of the UMD and IIFE wrappers, and the
// names are minified potentially causing collision with other globals.
// We use a regex to inject the helpers inside the wrappers.
// We don't need to create a MagicString here because both the helpers and
// the headers don't modify the sourcemap
const injectHelpers =
opts.format === 'umd'
? INJECT_HELPERS_UMD_RE
: opts.format === 'iife'
? INJECT_HELPERS_IIFE_RE
: undefined
if (injectHelpers) {
res.code = res.code.replace(
injectHelpers,
(_, helpers, header) => header + helpers,
)
}

@sun0day
Copy link
Member

sun0day commented Jan 31, 2023

@sun0day Yes, we are doing like that. I guess the regex is failing to catch these cases.

// #7188, esbuild adds helpers out of the UMD and IIFE wrappers, and the
// names are minified potentially causing collision with other globals.
// We use a regex to inject the helpers inside the wrappers.
// We don't need to create a MagicString here because both the helpers and
// the headers don't modify the sourcemap
const injectHelpers =
opts.format === 'umd'
? INJECT_HELPERS_UMD_RE
: opts.format === 'iife'
? INJECT_HELPERS_IIFE_RE
: undefined
if (injectHelpers) {
res.code = res.code.replace(
injectHelpers,
(_, helpers, header) => header + helpers,
)
}

OK. I'd like to dig into it and send a PR.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants