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: isBuiltin using patched native builtinModules #5827

Merged
merged 9 commits into from Nov 27, 2021

Conversation

Niputi
Copy link
Contributor

@Niputi Niputi commented Nov 24, 2021

Description

fix: #5826

Additional context


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.

@Niputi Niputi marked this pull request as ready for review November 24, 2021 22:32
@Shinigami92 Shinigami92 added feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority) labels Nov 25, 2021
packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
@Niputi
Copy link
Contributor Author

Niputi commented Nov 25, 2021

  • moved generation of list out of the function
  • removed adding old modules as wanted from Sodatea

@Niputi
Copy link
Contributor Author

Niputi commented Nov 25, 2021

looks like some node 12 tests expect fs/promises present

@ygj6
Copy link
Member

ygj6 commented Nov 26, 2021

looks like some node 12 tests expect fs/promises present

@Niputi Was the failed test created in #5248? I think it shows a clue as to why we need to count the missing built-in modules, which will solve problems like this and #4037

@patak-dev
Copy link
Member

I'm also not sure that what I said here isn't an issue: #5827 (comment).

For the beta, maybe we could go with the full harcoded list (or compute it by patching builtinModules, but making sure we have everything). We can use a set as suggested by Shini

const builtins = new Set([
  ...builtinModules,
  'fs/promises',
  // and others...
])

Later we can research with more time if it is safe to only use builtinModules (that looking at the linked issues, doesn't seems to be the case)

@sodatea
Copy link
Member

sodatea commented Nov 27, 2021

Yeah, the safe option, for now, is to use a hard-coded list + ...builtinModules so that it works with Node.js v12 to any future versions.

We can revisit it later in 2.8 or 3.0 to see if the edge case that "compiling using node v12 code to be run in node v16 in the server" is what we intend to support.
Let's add a TODO comment in the relevant code.

@patak-dev
Copy link
Member

For reference, it looks like @rollup/plugin-node-resolve directly uses node builtins
https://github.com/rollup/plugins/blob/02fb349d315f0ffc55970fba5de20e23f8ead881/packages/node-resolve/src/index.js#L14
We aren't using this plugin though, just another bit of info for when we analyze changing Vite later.

Co-authored-by: Shinigami <chrissi92@hotmail.de>
@patak-dev patak-dev changed the title fix: using native builtinModules array with added valid import syntaxes fix: isBuiltin using patched native builtinModules Nov 27, 2021
@patak-dev patak-dev merged commit 4a05a6e into vitejs:main Nov 27, 2021
@Niputi Niputi deleted the fix-5826 branch November 27, 2021 15:10
@willonzxy
Copy link

willonzxy commented Dec 7, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR can't resolve builtin modules that start with underscore
9 participants