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.

Sign up for GitHub

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

Jump to bottom

commonjs: Module ID containing forward slash is improperly normalized on Windows #923

Closed
pastelmind opened this issue Jul 3, 2021 · 0 comments · Fixed by #924
Closed

commonjs: Module ID containing forward slash is improperly normalized on Windows #923

pastelmind opened this issue Jul 3, 2021 · 0 comments · Fixed by #924

Comments

@pastelmind
Copy link
Contributor

  • Rollup version:
  • OS: Windows 10 Enterprise 20H2
  • Node.js version: 12.22.1 (x64)

When extracting the base name of a module from its module ID, @rollup/plugin-commonjs does not correctly normalize paths with separators that are not path.sep. This happens with some custom resolvers (e.g. Vite.js) which generate module IDs with forward slashes on Windows.

For example, if the module ID is C:/Users/Phil/Documents/GitHub/test-vite-app/node_modules/react/index.js, the plugin generates code like this on Windows:

var C__Users_Phil_Documents_GitHub_testViteApp_node_modules_react = { exports: {} };
var react_production_min = {};

This is undesireable because (1) it leaks the absolute path of the development machine into the bundle, and (2) it generates bundles with different hashes on different machines, hurting reproducible builds.

This is caused by getName(), which is used to extract the base name of a module:

plugins/packages/commonjs/src/utils.js

Lines 25 to 32 in 6867290

export function getName(id) {
const name = makeLegalIdentifier(basename(id, extname(id)));
if (name !== 'index') {
return name;
}
const segments = dirname(id).split(sep);
return makeLegalIdentifier(segments[segments.length - 1]);
}

Here, it uses path.sep to split the module ID. Since sep is \ on Windows, it fails to split paths that use /.

Since basename() and dirname() can handle both forward and backward slashes, I suggest that we replace lines 30-31 with:

  return makeLegalIdentifier(basename(dirname(id)));

See related investigation at vitejs/vite#2623 (comment)
@pastelmind pastelmind mentioned this issue Jul 3, 2021
Merged
9 tasks
pastelmind added a commit to Loathing-Associates-Scripting-Society/philter that referenced this issue Jul 3, 2021
@pastelmind
fix(manager): Create reproducible builds
4b2e15f 
Vite currently has a bug that causes it to generate different bundles on
Windows and Linux. This is caused by a fault in @rollup/plugin-commonjs
which is bundled into Vite.

We directly fix the fault in our `node_modules/` using patch-package.
Let's hope the Rollup devs accept my PR and the fix is integrated into
Vite.

More links:

- Bug reports:
    - vitejs/vite#2623
    - rollup/plugins#923
- Related pull requests:
    - rollup/plugins#924
@pastelmind pastelmind mentioned this issue Jul 3, 2021
Merged
pastelmind added a commit to Loathing-Associates-Scripting-Society/philter that referenced this issue Jul 4, 2021
@pastelmind
fix(manager): Create reproducible builds
551ebe0 
Vite currently has a bug that causes it to generate different bundles on
Windows and Linux. This is caused by a fault in @rollup/plugin-commonjs
which is bundled into Vite.

We directly fix the fault in our `node_modules/` using patch-package.
Let's hope the Rollup devs accept my PR and the fix is integrated into
Vite.

More links:

- Bug reports:
    - vitejs/vite#2623
    - rollup/plugins#923
- Related pull requests:
    - rollup/plugins#924
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(commonjs)!: Correctly infer module name for any separator
1 participant
@pastelmind