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: Add vite framework plugin if not found #19259

Merged
merged 7 commits into from
Oct 4, 2022
Merged

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Sep 26, 2022

Issue: storybookjs/builder-vite#498

Closes #19245

What I did

When we made the change in 7.0 to start using the user's vite.config.js, I made the (faulty) assumption that users would already have vite framework plugins (e.g. @vitejs/plugin-react) installed. But this isn't always true, for example, a project might just be a collection of components being exported, and there may not be an actual app.

So, this PR checks to see if the required framework plugin is already in the config, and if not, adds it. I created a utility function to check for the plugin, but ideally this would live in a vite-core instead of duplicated across each framework. This, combined with the duplication in #19216, makes me lean towards creating such a package. But, it can be done any time and these duplications cleaned up at that point, so I don't think it's a blocker.

How to test

Create a sandbox, delete vite.config.js, and start storybook. Without this change there would be a crash, but it should work just fine now.

@IanVS IanVS changed the title Add react vite plugin if not found Vite: Add vite framework plugin if not found Sep 26, 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.

WFM if you want to merge, or:

  • consider adding to viteFinal instead
  • consider adding an automigration that prompts & can add for you on your behalf if you opt in

# Conflicts:
#	code/frameworks/react-vite/package.json
#	code/frameworks/vue3-vite/package.json
@IanVS
Copy link
Member Author

IanVS commented Sep 27, 2022

I've given it some more thought, and I think this approach is the best one for now because:

  • I can't print a warning from the framework after checking for a plugin in the config, since the user's viteFinal hasn't run at that point, and adding framework-specific logic into builder-vite is a road to tears.
  • If there's one thing I've learned in the last year or so of maintaining builder-vite, it's that users want things to "just work". I think in this case, there's no real benefit to forcing users to add a plugin to their viteFinal, when we can just do it for them, and they're still free to remove the plugin and add their own in viteFinal if they have an advanced use-case.

@shilman
Copy link
Member

shilman commented Sep 28, 2022

@IanVS Any idea what's going on with CI here?

@IanVS
Copy link
Member Author

IanVS commented Sep 28, 2022

Nope, it looks like the webpack version of vue might be missing a babel dependency … for react? I have a hard time imagining how that would be related to this change. 🤔

@IanVS
Copy link
Member Author

IanVS commented Sep 28, 2022

It looks like #19243 is also experiencing the same failure in CI.

# Conflicts:
#	code/frameworks/react-vite/src/preset.ts
@IanVS IanVS mentioned this pull request Oct 3, 2022
3 tasks
@IanVS IanVS merged commit 2674cc1 into next Oct 4, 2022
@IanVS IanVS deleted the vite/framework-plugins branch October 4, 2022 17:47
@newtriks
Copy link

newtriks commented Oct 5, 2022

@IanVS I'm not sure if this is related to this PR, however, I get WARN unable to find package.json for @vitejs/plugin-react when running storybook now? Looking in node_modules I can in fact see the package.json.

vite.config.js

import { defineConfig } from 'vite'
import react from '@vitejs/plugin-react'

// https://vitejs.dev/config/
export default defineConfig({
  plugins: [react()]
})

@IanVS
Copy link
Member Author

IanVS commented Oct 5, 2022

Hi @newtriks, this PR hasn't been released yet, so it's not from this. In fact, it's a common warning that I see for many packages in 7.0. I've asked about it in discord: https://discord.com/channels/486522875931656193/915642585761075280/1024397115272740934, but not gotten an answer. For now, I think it's safe to ignore.

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