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 migration #17860

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Vite migration #17860

wants to merge 17 commits into from

Conversation

Neues
Copy link
Contributor

@Neues Neues commented Oct 23, 2024

Closes #

Replaces Storybook Webpack builder with Vite, providing faster startup and refresh times.

Changelog

New

  • {{new thing}}

Changed

  • {{change thing}}

Removed

  • {{removed thing}}

Testing / Reviewing

  • yarn install
  • yarn build, and if this fails, yarn rebuild
  • cd packages/react
  • yarn storybook

* begin to replace webpack with vite - updating story book config
* add vite configuration in storybook
* update storybook dependencies
* remove console logs, reinstall and rebuild
* revert changes to feature flag
* uninstall vite-tsconfig-paths which was unnecessary
* remove comments
* revert changes to feature flag
* uninstall vite-tsconfig-paths which was unnecessary
* remove comments
* fix issue with @storybook/addon-essentials docs overriding remarkGfm
Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit 5aa82fe
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/6764708c4f490a00088cbe44
😎 Deploy Preview https://deploy-preview-17860--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 91ab138
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/671a39edea50d4000881dee6
😎 Deploy Preview https://deploy-preview-17860--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 5aa82fe
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6764708c917d740008a70ffc
😎 Deploy Preview https://deploy-preview-17860--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@guidari
Copy link
Contributor

guidari commented Oct 23, 2024

Hey @Neues
Here it is a issue for this migration. We already tried to start a couple months ago as well, so you might find code in the old commits of this PR
Until this commit it was using vite.

Neues added 2 commits October 24, 2024 13:04
* remove unused style config
* revert change to storybook accessibility checker library
@Neues
Copy link
Contributor Author

Neues commented Oct 24, 2024

Hey @Neues Here it is a issue for this migration. We already tried to start a couple months ago as well, so you might find code in the old commits of this PR Until this commit it was using vite.

Hi @guidari, thanks for having a look at this already. I was referencing the work you linked as I was drafting this up. I don't think I'm experiencing the same problems that you were with the __DEV__ global, but I'm not as familiar with it. I don't see errors in console or on the storybook UI about __DEV__ being undefined. However, in ModalWrapper for example, I do not see the warning in console that is meant to be produced here. If I move this check into the render, I start seeing it. I also see it in the console when looking at unstable__StaticNotification as expected.

@tay1orjones tay1orjones added the type: infrastructure 🤖 Issues relating to devops, tech debt, etc. label Dec 4, 2024
@tay1orjones tay1orjones added this to the 2024 Q4 milestone Dec 4, 2024
@guidari
Copy link
Contributor

guidari commented Dec 4, 2024

Hey @Neues
I just put this PR on my sprint work. I'm gonna take a look at it so we can finish this migration together!

@guidari
Copy link
Contributor

guidari commented Dec 17, 2024

I'm seeing this error when running yarn storybook in this PR. Is it the same one you are experience?

Screenshot 2024-12-17 at 14 34 49

@Neues
Copy link
Contributor Author

Neues commented Dec 18, 2024

I'm seeing this error when running yarn storybook in this PR. Is it the same one you are experience?

Hi @guidari, no, I was able to run this locally and I wasn't seeing any errors when doing so. Try running yarn build or yarn rebuild after running yarn install

@guidari
Copy link
Contributor

guidari commented Dec 19, 2024

Weird, mine isn't working 🤔
If you could to solve those conflicts I can try to pull again to check it out.

@Neues
Copy link
Contributor Author

Neues commented Dec 19, 2024

Ok yeah, I also saw the issue after merging in from main. I found this in the FAQ and was able to get it working.
https://storybook.js.org/docs/faq#how-do-i-fix-module-resolution-in-special-environments

I bumped the versions up to latest as well, should be using vite@6 now. It was running for me locally.

@guidari
Copy link
Contributor

guidari commented Dec 19, 2024

Cool! I was able to run Storybook without errors. I also ran yarn dedupe and yarn format and pushed a change to prevent unintended modifications.

I guess we just have to figure it out why the building isn't working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: infrastructure 🤖 Issues relating to devops, tech debt, etc.
Projects
Status: ⏱ Backlog
Development

Successfully merging this pull request may close these issues.

3 participants