fix(commonjs)!: Correctly infer module name for any separator #924
Conversation
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
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
|
I added a test. Unfortunately, it works only on Windows, where replacing backslashes with slashes is perfectly fine. On Linux, the backslash is treated as a regular character, which means that we'd get an error if we replaced slashes with backslashes. |
shellscape
force-pushed
the
master
branch
13 times, most recently
from
486cc69 to
34ba4ce
Compare
|
@danielgindi I apologize for pinging you, but this PR hasn't even got a chance to run workflows yet. Could you please take a look? |
|
@danielgindi Thanks for letting the CI workflow happen, but it failed and I'm not sure why. The log is at https://github.com/rollup/plugins/runs/3150391648, here's an excerpt:
I rebased the commit onto the current |
Add test for module IDs that use a path separator that is different from `path.sep`. Previously, when a module whose file name is `index.js` was resolved to a module ID containing different path separators, the commonjs plugin generated a variable name containing the module's absolute path.
BREAKING CHANGES: Correctly infer the module name from the module ID, regardless of the path separator used in the module ID and the value of `path.sep`. This generates variable names like `react` instead of `C__testViteApp_node_modules_react` when a module ID of `C:/test-vite-app/node_modules/react/index.js` is given on Windows.
shellscape
requested review from
benmccann,
danielgindi,
guybedford,
LarsDenBakker,
lukastaegert and
tjenkinson
|
@pastelmind looks like your tests all pass now. I've requested review from a few folks to get eyes on this for you. |
|
To me this looks much more resilient than the current code. Using core path functions handles this stuff for us. |
|
|
Rollup Plugin Name:
commonjsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: #923
Description
When a module whose file name is
index.jsis resolved to a module ID containing a directory separator that is different frompath.sepof the current platform, the commonjs plugin incorrectly infers the base name of the module. This causes the absolute path of the module to be embedded into the bundle, and prevents generating reproducible builds across different machines.For example, with a module ID of
C:/Users/Phil/Documents/GitHub/test-vite-app/node_modules/react/index.json Windows, the plugin generates code like this:This PR fixes the problem by using
path.basename()to extract the final path component instead of splitting the string withpath.sep.With the fix, the plugin generates code like this:
Closes #923