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

fix: prefer exports when resolving #10371

Merged
merged 3 commits into from Oct 13, 2022

Conversation

benmccann
Copy link
Collaborator

Description

Closes #10352

Additional context

Svelte has a package.json in each directory to support older versions of Node since exports is only present in Node 12 and newer. Vite is preferring these nested package.json files over the exports field which is incorrect

The 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Oct 7, 2022
@benmccann benmccann force-pushed the exports-legacy-fallback branch 2 times, most recently from 6ce0484 to 741e1ec Compare October 12, 2022 21:01
Copy link
Member

@bluwy bluwy left a 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!

@patak-dev patak-dev enabled auto-merge (squash) October 13, 2022 07:33
@patak-dev patak-dev merged commit 3259006 into vitejs:main Oct 13, 2022
})!

if (!pkg) {
const rootPkgId = possiblePkgIds[0]
Copy link
Member

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.

Copy link
Collaborator Author

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

@aleclarson
Copy link
Member

aleclarson commented Nov 13, 2022

I created a test project since I was having a hard time interpreting the spec. What I found is that it will only use exports from the package.json in the package root, but it will use the package.json in subdirectories to determine whether to interpret the file as ESM or CJS, which sounds like what you said.
– Ben McCann (#10371 (comment))

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.

@aleclarson
Copy link
Member

Svelte has a package.json in each directory to support older versions of Node since exports is only present in Node 12 and newer. Vite is preferring these nested package.json files over the exports field which is incorrect

What issue does this cause? Can't you just keep the main/module fields in each nested package.json aligned with the exports field from the top-level package.json?

@aleclarson
Copy link
Member

@benmccann Does Node.js only check the "root package" for an exports field, or does it search in each parent directory of the import ID and stop at the first exports field it finds?

@benmccann
Copy link
Collaborator Author

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.

I don't think Node could change its behavior and randomly break working packages.

What issue does this cause?

Vite was resolving to the wrong file

Can't you just keep the main/module fields in each nested package.json aligned with the exports field from the top-level package.json?

They are

Does Node.js only check the "root package" for an exports field, or does it search in each parent directory of the import ID and stop at the first exports field it finds?

If I recall correctly, I believe it only checks the root package

@aleclarson
Copy link
Member

What issue does this cause?

Vite was resolving to the wrong file

Can't you just keep the main/module fields in each nested package.json aligned with the exports field from the top-level package.json?

They are

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?

@benmccann
Copy link
Collaborator Author

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.

@benmccann
Copy link
Collaborator Author

I went back and looked at the original bug report and it refreshed my memory.

The root package.json looks like:

    "./internal": {
      "types": "./types/runtime/internal/index.d.ts",
      "import": "./internal/index.mjs",
      "require": "./internal/index.js"
    },

And then the sub-directory or sub-package or whatever you want to call it looks like:

{
  "main": "./index",
  "module": "./index.mjs",
  "types": "./index.d.ts"
}

I believe the issue here is that the module field is only used when the package is in ssr.noExternal. I'm thinking now that we probably could add an exports map to the Svelte sub-package as a workaround. But that's really just a workaround and it's better to fix Vite.

@bluwy
Copy link
Member

bluwy commented Nov 14, 2022

Yup, to add on Ben's explanation, Node.js treats exports differently, that if the exports is define in the root package.json, it should respect that instead of any subdirectories' package.json. Previously Vite was checking closest package.json regardless which deviates from Node.js resolution algorithm. https://nodejs.org/api/esm.html#resolver-algorithm-specification (PACKAGE_RESOLVE no11 > no5, where packageSpecifier doesn't include the import subpath)

@aleclarson
Copy link
Member

Node.js treats exports differently, that if the exports is define in the root package.json, it should respect that instead of any subdirectories' package.json.

Ah yes, that looks to be correct, now that I look at the spec.

PACKAGE_RESOLVE

9. Let selfUrl be the result of PACKAGE_SELF_RESOLVE(packageName, packageSubpath, parentURL).

That PACKAGE_SELF_RESOLVE call runs before the parent directories are searched, so the root exports will take precedence as Ben stated. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR resolve always happens as require even when it's an import
4 participants