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] rootDir causes commonjs modules to resolve incorrectly #2231

Open
lucaelin opened this issue Apr 20, 2023 · 2 comments · May be fixed by #2727
Open

[commonjs] rootDir causes commonjs modules to resolve incorrectly #2231

lucaelin opened this issue Apr 20, 2023 · 2 comments · May be fixed by #2727

Comments

@lucaelin
Copy link
Contributor

I've recently updated a project of mine to use the latest dev-server packages, those that include the commonjs and rollup3 fixes.
While doing so, I noticed an issue with commonjs-modules appearing as their absolute file-system paths in the browsers network tab. Upon investigation, I realized that this is somehow related to setting the rootDir option. Removing this options from the config fixes the import.

The broken imports appear to resolve "too far up" in the file system, walking outside of the projects folder, strangely the working non-commonjs imports also seem to resolve "too far up" , but end up working fine.

I've managed to reproduce the issue in this branch:
https://github.com/lucaelin/rollup-commonjs-bug/tree/rootDir-commonjs
@justinfagnani
Copy link
Contributor

I have this issue too. Standard imports work, but as soon as I added plugin-commonjs, require() broke with invalid absolute paths.

I tracked it down to some faulty logic in the rollup adapter that's handling the null-byte paths and resolving the paths to be absolute - that logic isn't taking into account the rootDir at all. While I don't think they need to be turned into absolute paths, the fix to them is simple:

The problem is here:

web/packages/dev-server-rollup/src/rollupAdapter.ts

Line 272 in 8c0250c

resolvedImportPath = `\0${path.resolve(`${upDirs}${matches[2]}`)}`;

The fix is to use rootDir:

if (matches) {
    const upDirs = new Array(parseInt(matches[1], 10)).fill('..');
    resolvedImportPath = `\0${path_1.default.resolve(rootDir, ...upDirs, matches[2])}`;
}

I'll put together a PR

@justinfagnani justinfagnani linked a pull request May 10, 2024 that will close this issue
Draft
@justinfagnani
Copy link
Contributor

For tests, presumably we want something in here: https://github.com/modernweb-dev/web/tree/master/packages/dev-server-rollup/test/node/fixtures/commonjs

To be set up with a root dir. Anyone know how I can make a server configuration just for one test?

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 rollupAdapter handling of outside-workspace imports justinfagnani/web
2 participants
@justinfagnani @lucaelin