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: specify own Node version as target when bundling config files #17307

Merged
merged 13 commits into from
Jul 30, 2024

Conversation

SegaraRai
Copy link
Contributor

@SegaraRai SegaraRai commented May 24, 2024

Description

fixes #17291

I am uncertain if it is safe to specify node${process.versions.node} for target.
At least esbuild supports a notation like node22.2.0 and node99.999.9999, but I am not sure about the following points:

  • Whether process.versions.node is always present and in X.Y.Z format (at least bun seemed to have this)
  • Compatibility between esbuild and Node versions

Copy link

stackblitz bot commented May 24, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

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.

This looks fine to me. I remember discussing with @patak-dev that we could derive from process.versions.node but decided otherwise to prevent potential compat issues. The linked issue seems to be a good argument to derive it still.

playground/config/packages/entry/json.ts Outdated Show resolved Hide resolved
@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 27, 2024
@SegaraRai
Copy link
Contributor Author

SegaraRai commented Jul 24, 2024

I have noticed that the tests in this PR are not working (dynamic imports are not modified by esbuild at all!), and I am wondering what I should do about it.
Perhaps I should change I have changed to static import since all Node.js in the current CI support import attributes, but is it acceptable for the tests not to pass in the old (<= v18.19.x, <= v20.9.x) Node.js?

@SegaraRai SegaraRai requested a review from bluwy July 24, 2024 16:43
@SegaraRai
Copy link
Contributor Author

CI failed, but I think it will succeed on re-run (I don't have permission to do that).

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.

but is it acceptable for the tests not to pass in the old (<= v18.19.x, <= v20.9.x) Node.js?

Could we add some node version checks so that we skip testing this case in older versions, and so it doesn't fail for devs using an older node version locally? (I think I saw some checks you did in the previous commits)

packages/vite/src/node/config.ts Outdated Show resolved Hide resolved
@bluwy bluwy added this to the 5.4 milestone Jul 25, 2024
@SegaraRai
Copy link
Contributor Author

SegaraRai commented Jul 25, 2024

By splitting the test file, the import attributes test can now be run conditionally.
I wanted to use dynamic imports in the test files for feature detection, but this was not possible because all imports in the test code are rewritten by Vitest (Vite).

Tested with v18.12.0, v18.19.0, v18.20.0, v20.12.0, and v22.0.0.
From v20.0.0 to v20.11.0, for unknown reasons, a problem occurred in another part of the system and the test could not be run.

@SegaraRai SegaraRai requested a review from bluwy July 25, 2024 17:14
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.

Looks great! Thanks for updating the PR. I think the test for now is fine and should be enough.

@bluwy
Copy link
Member

bluwy commented Jul 26, 2024

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 051c2ac: Open

suite result latest scheduled
analogjs failure success
astro failure failure
nuxt failure failure
vike failure failure
vite-plugin-react-pages failure failure
vitest failure failure

histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, sveltekit, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

@bluwy
Copy link
Member

bluwy commented Jul 26, 2024

The analog fail seems to be something on their end. I think this is ready to go in the next minor.

@bluwy bluwy merged commit bbf001f into vitejs:main Jul 30, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Cannot import external JSON in vite.config.ts on Node v22
3 participants