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

chore: use node prefix #8309

Merged
merged 8 commits into from Jun 19, 2022
Merged

chore: use node prefix #8309

merged 8 commits into from Jun 19, 2022

Conversation

Shinigami92
Copy link
Member

Description

Replace all node imports with node:*

Additional context

Motivation: #8305 (comment)


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.

@Shinigami92 Shinigami92 self-assigned this May 24, 2022
@Shinigami92 Shinigami92 added the p1-chore 🧹 Doesn't change code behavior (priority) label May 24, 2022
@Shinigami92 Shinigami92 marked this pull request as ready for review May 24, 2022 13:29
@sapphi-red
Copy link
Member

sapphi-red commented May 24, 2022

node prefixes with require are only supported with node 14.18.0+, 16.0.0+.
But Vite and plugins supports 14.6.0+.

"node": ">=14.6.0"

So it might need to strip it in build like tsup does or bump the support version to 14.18.0. Maybe unbuild handles this already?

@Shinigami92 Shinigami92 marked this pull request as draft May 24, 2022 13:38
@Shinigami92 Shinigami92 marked this pull request as ready for review May 24, 2022 14:18
@Shinigami92
Copy link
Member Author

node prefixes with require are only supported with node 14.18.0+, 16.0.0+. But Vite and plugins supports 14.6.0+.

"node": ">=14.6.0"

So it might need to strip it in build like tsup does or bump the support version to 14.18.0. Maybe unbuild handles this already?

We could discuss this in the next team meeting 🤔

@bluwy
Copy link
Member

bluwy commented May 24, 2022

So it might need to strip it in build like tsup does or bump the support version to 14.18.0. Maybe unbuild handles this already?

unbuild doesn't handle unfortunately. It externalises node: and builtinModules by default, which makes it not possible for us to change the id. Setting the rollup.esbuild.target: 'node14.6' config doesn't work too.

For Vite core we have control of the build so it's likely possible with some grease. Otherwise, the easiest way seems to be bumping the node version, though I'm not sure if internal code cleanup justifies bumping 🤔

@patak-dev
Copy link
Member

I vote to wait here, and add this to the reasons to bump (we're surely going to find more in the next months)

@sapphi-red
Copy link
Member

I found that Vite is using fs.rmSync which requires Node v14.14.0+.

@bluwy
Copy link
Member

bluwy commented Jun 15, 2022

Another trick would be to create a Rollup plugin to resolve node: imports and strips the protocol part, so that the final output remains not having the protocol. Even if we bump up to node 14.18, it'd help dedupe node:url and url, since non-protocol imports can happen in deps.

We should probably bump to 14.14 though per sapphi's comment.

EDIT: I just realized sapphi mentioned that tsup did this with an esbuild plugin too.

@Shinigami92 Shinigami92 added the needs rebase There's a merge conflict label Jun 15, 2022
@sapphi-red sapphi-red added the p2-to-be-discussed 🍰 Enhancement under consideration (priority) label Jun 16, 2022
@bluwy
Copy link
Member

bluwy commented Jun 19, 2022

Rebased this.

  1. Version upgrade is done at feat!: bump minimum node version to 14.18.0 #8662
  2. Didn't add a rollup plugin to dedupe builtin imports, instead opened feat(node-resolve): dedupe preferBuiltins protocol rollup/plugins#1212
  3. Thought to add an eslint plugin to ensure node:, but it seems to only exists in eslint-plugin-unicorn. Not sure if we should add the whole plugin just for one rule, was hoping eslint-plugin-import to support it.

@patak-dev patak-dev merged commit 60721ac into main Jun 19, 2022
12 checks passed
@patak-dev patak-dev deleted the use-node-prefix branch June 19, 2022 19:59
@sapphi-red sapphi-red mentioned this pull request Aug 3, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase There's a merge conflict p1-chore 🧹 Doesn't change code behavior (priority) p2-to-be-discussed 🍰 Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants