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

ensure we don't resolve modules in parent folders #158

Open
rchl opened this issue Mar 14, 2023 · 6 comments
Open

ensure we don't resolve modules in parent folders #158

rchl opened this issue Mar 14, 2023 · 6 comments
Labels
breaking change bug Something isn't working

Comments

@rchl
Copy link
Contributor

rchl commented Mar 14, 2023

I've stumbled upon a crash trying to start a Nuxt 3-based project in a subdirectory of a Nuxt 2 project.

So the main Nuxt 2 project at /project/ and a Nuxt 3-based docs subdirectory at /project/docs. This crashes due to nuxt dependency from the root project getting picked up.

It seems the issue is in this code:

mlly/src/resolve.ts

Lines 75 to 79 in 6327ad8

new URL("./", url),
// If url is directory
new URL(joinURL(url.pathname, "_index.js"), url),
// TODO: Remove in next major version?
new URL("node_modules", url)

This _resolve function receives an id parameter with a nuxt value.
It creates url with the value of the current working directory so /project/docs.
Then new URL("./", url) actually resolves to the /project/ directory instead of /project/docs which seems very much not expected to me.

Try this in a browser:

new URL('./', 'file:///project/docs').href
"file:///project/"

(Same for the new URL("node_modules", url) line above which seems to be scheduled for removal at some point)

@rchl
Copy link
Contributor Author

rchl commented Mar 14, 2023

BTW. The chain of calls that leads to this code is:

nuxi (loadKit)
->
@nuxt/kit (loadNuxt)
->
pkg-types (resolvePackageJSON)
->
mlly (resolvePath)
->
mlly (_resolve)

@rchl
Copy link
Contributor Author

rchl commented Mar 14, 2023

Hmm, there is more issues like that elsewhere. For example in getPackageScopeConfig which I think comes from internal Node libs. So I think the solution to issues like that is ensure that the base URL ends in slash if it's a directory.

new URL('./', 'file:///project/docs/').href
"file:///project/docs/"

@rchl
Copy link
Contributor Author

rchl commented Mar 14, 2023

This rabbit hole goes deep.
The first directory that is involved and that doesn't include the trailing slash is the rootDir one resolved in https://github.com/nuxt/nuxt/blob/184d57bb1923b8bf21a372eabdb67e1241d2023f/packages/nuxi/src/commands/dev.ts#L41.
Forcing trailing slash here fixes this specific issue but creates another.
So it's no longer clear to me what and where should be fixed.
It seems to me like using URI for resolving directory paths is a little brittle since it's not clear whether path is to a directory or a file without doing file IO.

@pi0
Copy link
Member

pi0 commented Mar 15, 2023

Hi dear @rchl. Thanks for sharing all investigations.

Yes, that's kinda tricky situation. We removed the directory URL workaround but it introduced other regressions in Nuxt 3.

I think first good step would be making a minimal mlly reproduction to test the behavior if you can help 🙏🏼 . We might try to introduce a major version on progressively adopt it within nuxt ecosystem.

@rchl
Copy link
Contributor Author

rchl commented Mar 15, 2023

I think first good step would be making a minimal mlly reproduction to test the behavior if you can help 🙏🏼

I'm thinking that a unittest for that would be a good reproduction?
So something like https://github.com/unjs/mlly/blob/main/test/fixture/resolve.mjs but I guess this is just a lone file that is not really testing anything?

@pi0
Copy link
Member

pi0 commented Mar 15, 2023

Yeah. We probably could make an external repo for it. Especially trying node_modules resolution too via an exclude in .gitignore and then moving it to fixtures (because previous regression happened this way).

@pi0 pi0 changed the title Incorrect module path resolving ensure we don't resolve modules in parent folders Jan 11, 2024
@pi0 pi0 added bug Something isn't working breaking change labels Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants