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

Vite: Make vite a peer dependency, update plugins #20281

Merged
merged 14 commits into from
Dec 16, 2022
Merged

Vite: Make vite a peer dependency, update plugins #20281

merged 14 commits into from
Dec 16, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Dec 14, 2022

Issue:

Closes #20227
Closes #20214

What I did

This moves vite to a peer dependency, and allows either version 3 or 4.

It keeps the plugins as normal dependencies. They are only used as fallbacks in case the user does not have a vite.config.js. For the most part, the newer plugins will work with vite 3, with the possible exception of svelte. But the solution in those cases is pretty straightforward (update vite or install an older plugin and use a vite.config.js).

As noted in #20227, we may want to add a migration step to add vite, but we don't need to add one for the plugins, if we go with this approach.

How to test

CI should pass for all vite frameworks.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes looks good, but we should probably add an entry to MIGRATION.md

We might also want an automigration in this PR? I don't know.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that, vite was already a peer dep in 6.5, no migration nor automigration needed I think

@IanVS
Copy link
Member Author

IanVS commented Dec 15, 2022

I need to add an eslint ignore because the svelte plugin does not have a main in package.json.

# Conflicts:
#	code/frameworks/react-vite/package.json
#	code/frameworks/svelte-vite/package.json
#	code/frameworks/vue3-vite/package.json
#	code/yarn.lock
@@ -9,6 +9,8 @@ export const core: StorybookConfig['core'] = {

export const viteFinal: NonNullable<StorybookConfig['viteFinal']> = async (config, options) => {
const { plugins = [] } = config;
// TODO: set up eslint import to use typescript resolver
// eslint-disable-next-line import/no-unresolved
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndelangen
Copy link
Member

#20294 does something similar 👍

code/package.json Show resolved Hide resolved
code/frameworks/svelte-vite/package.json Outdated Show resolved Hide resolved
code/lib/builder-vite/package.json Outdated Show resolved Hide resolved
code/frameworks/react-vite/package.json Outdated Show resolved Hide resolved
@IanVS
Copy link
Member Author

IanVS commented Dec 16, 2022

@ndelangen it looks like during one of the merges of next, we lost the fix for pinning react-docgen. I've re-applied it here, and 🤞 that CI will be green now.

@socket-security
Copy link

Socket Security Pull Request Report

Dependency issues detected. If you merge this pull request, you will not be alerted to the instances of these issues again.

📜 Install scripts

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Package Script field Source
es5-ext@0.10.62 (added) postinstall code/package.json via @compodoc/compodoc@1.1.19, pdfmake@0.2.6, @foliojs-fork/linebreak@1.1.1, brfs@2.0.2, static-module@3.0.4, scope-analyzer@2.1.2, es6-map@0.1.5,scripts/package.json via @compodoc/compodoc@1.1.19, pdfmake@0.2.6, @foliojs-fork/linebreak@1.1.1, brfs@2.0.2, static-module@3.0.4, scope-analyzer@2.1.2, es6-map@0.1.5
🧌 Protestware/Troll package

This package is a joke, parody, or includes undocumented or hidden behavior unrelated to its primary function.

Package Note Source
es5-ext@0.10.62 (added) This package prints a protestware console message on install regarding Ukraine for users with Russian language locale code/package.json via @compodoc/compodoc@1.1.19, pdfmake@0.2.6, @foliojs-fork/linebreak@1.1.1, brfs@2.0.2, static-module@3.0.4, scope-analyzer@2.1.2, es6-map@0.1.5,scripts/package.json via @compodoc/compodoc@1.1.19, pdfmake@0.2.6, @foliojs-fork/linebreak@1.1.1, brfs@2.0.2, static-module@3.0.4, scope-analyzer@2.1.2, es6-map@0.1.5
Pull request report summary
Issue Status
Install scripts ⚠️ 1 issue
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ⚠️ 1 issue
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

  • @SocketSecurity ignore es5-ext@0.10.62

Powered by socket.dev

# Conflicts:
#	code/frameworks/sveltekit/package.json
#	code/yarn.lock
@shilman shilman changed the title Vite: make vite a peer dependency, update plugins Vite: Make vite a peer dependency, update plugins Dec 16, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @IanVS do we need to do a followup with some combination of:

  • Documentation updates
  • Automigration to automatically add a vite dep if you don't have it

@IanVS
Copy link
Member Author

IanVS commented Dec 16, 2022

@shilman we could, though I'm not entirely convinced it's necessary. I think it makes sense to have the migration for react, since it's surprising to need react in a svelte project, but it's not surprising to need vite in a svelte-vite project. Also, in 6.5, vite was a peer dependency, so this is not a change in 7.0. My personal recommendation would be to avoid making our already-very-long mirgration doc even longer, and wait to see if we hear any confusion over it. Personally I don't think we will.

@shilman
Copy link
Member

shilman commented Dec 16, 2022

@IanVS WFM! Merging 🎉

@shilman shilman merged commit 1666a69 into next Dec 16, 2022
@shilman shilman deleted the vite/peer-dep branch December 16, 2022 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants