-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
fix: prefer exports when resolving #10371
Conversation
6ce0484
to
741e1ec
Compare
741e1ec
to
043be4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
})! | ||
|
||
if (!pkg) { | ||
const rootPkgId = possiblePkgIds[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is actually the root package ID, unless you mean something different than "the @foo/bar
part of @foo/bar/x/y/z
".
Reason being, the possiblePkgIds
array is reversed in the statement before this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're probably right about that
It seems dangerous to infer Node's intended behavior from its implementation instead of its specification, as the observed behavior could be unintended and fixed in Node later on without us the wiser. |
What issue does this cause? Can't you just keep the main/module fields in each nested |
@benmccann Does Node.js only check the "root package" for an |
I don't think Node could change its behavior and randomly break working packages.
Vite was resolving to the wrong file
They are
If I recall correctly, I believe it only checks the root package |
These two answers seem to conflict, or am I missing something? How was Vite resolving to the wrong file if both the root and nested package.json point to the same file? |
I can't recall anymore. I'd have to go back to an old version of Vite and run it against Svelte again. That being said, I'm not sure the answer would change much. This is part of the Node resolution algorithm and is something we should probably support regardless. |
I went back and looked at the original bug report and it refreshed my memory. The root
And then the sub-directory or sub-package or whatever you want to call it looks like:
I believe the issue here is that the |
Yup, to add on Ben's explanation, Node.js treats |
Ah yes, that looks to be correct, now that I look at the spec.
That |
Description
Closes #10352
Additional context
Svelte has a
package.json
in each directory to support older versions of Node sinceexports
is only present in Node 12 and newer. Vite is preferring these nestedpackage.json
files over theexports
field which is incorrectThe code was also assuming all modules are required, which is wrong since it uses a dynamic import for everything
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).