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.

Sign up for GitHub

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

Jump to bottom

Build: Ignore CI version range of vite & log install during CI #24935

Closed
wants to merge 8 commits into from
Closed

Build: Ignore CI version range of vite & log install during CI #24935

wants to merge 8 commits into from

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Nov 21, 2023 •
edited
Loading

What I did

  • pipe the output of packageManager's install sub-process in CI
  • ignore the vite version peerDependency conflict in CI during the creation of sandboxes

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@ndelangen ndelangen added build Internal-facing build tooling & test updates ci:daily Run the CI jobs that normally run in the daily job. labels Nov 21, 2023
@ndelangen ndelangen self-assigned this Nov 21, 2023
@socket-security Socket Security
Copy link

socket-security bot commented Nov 21, 2023 •
edited
Loading

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: @vitejs/[email protected]

valentinpalkovic
valentinpalkovic requested changes Nov 21, 2023
View reviewed changes
code/frameworks/react-vite/package.json Outdated
@@ -51,7 +51,7 @@
"@rollup/pluginutils": "^5.0.2",
"@storybook/builder-vite": "workspace:*",
"@storybook/react": "workspace:*",
"@vitejs/plugin-react": "^3.0.1",
"@vitejs/plugin-react": "^3.0.1 || ^4.2.0",
Copy link
Contributor

@valentinpalkovic valentinpalkovic Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bit more complex, unfortunately.
@vitejs/plugin-react@4 requires Vite 4.2, but we still support Vite 3. If we define it like this, the latest version is always installed. See the discussion at #24901

Copy link
Member Author

Choose a reason for hiding this comment

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

Please tell me what to do, right now this is my only option to getting the CI green.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe @IanVS has some advice?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to bundle it in now, that way the mismatching peerDep thing won't be a surface.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a yarn resolution/override for CI? That is what we have to tell our users to do to avoid the peer dependency warning as well, until we can drop support for vite < 4.2 (in SB 8 perhaps?).

The other option we could consider is adopting a bit of a "semver-lite" approach, whereby we allow some types of small breaking changes during minor releases, like dropping support for old versions of vite when using the fallback plugins.

I'm not sure that bundling will work, because it needs to be installed in a way that vite can find it, right?

@ndelangen ndelangen changed the title Build: Fix CI version range of vite & log install during CI Build: Ignore CI version range of vite & log install during CI Nov 21, 2023
IanVS
IanVS reviewed Nov 21, 2023
View reviewed changes
code/builders/builder-vite/input/iframe.html
@@ -28,6 +28,8 @@
// doesn't fail (it will simply be disabled)
window.module = undefined;
window.global = window;
// prevent: "ReferenceError: process is not defined"
window.process = window.process || {};
Copy link
Member

Choose a reason for hiding this comment

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

The package manager cli code shouldn't be running in the iframe, should it?

I would not want to merge this PR with this change, but I think you're doing it just for troubleshooting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually...

I am running into this:
https://app.circleci.com/pipelines/github/storybookjs/storybook/64114/workflows/6ea5ea73-052c-4872-8329-316db3da94c3/jobs/598100

To be specific:
https://6308736456ad2046275c0ae7-hzqlbitzzd.chromatic.com

To be even more specific:
https://6308736456ad2046275c0ae7-hzqlbitzzd.chromatic.com/?path=/story/stories-renderers-svelte-args--remount-on-reset-story-args

process is not defined
ReferenceError: process is not defined
    at https://6308736456ad2046275c0ae7-hzqlbitzzd.chromatic.com/assets/index-cmme_mpW.js:395:7384

I tried bundling in the plugin, and that resulted in that error, but even after reverting that, I'm still seeing that.
I think it might just be how vite5 works?

So, I'm trying anything I can to get CI green. Been trying all day.

Copy link
Member Author

Choose a reason for hiding this comment

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

And it's still broken, even with this...

@IanVS IanVS mentioned this pull request Nov 21, 2023
Merged
8 tasks
@ndelangen ndelangen closed this Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IanVS IanVS IanVS left review comments

@valentinpalkovic valentinpalkovic valentinpalkovic requested changes

@yannbf yannbf Awaiting requested review from yannbf

@kasperpeulen kasperpeulen Awaiting requested review from kasperpeulen

@JReinhold JReinhold Awaiting requested review from JReinhold

Assignees

@ndelangen ndelangen

Labels
build Internal-facing build tooling & test updates ci:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ndelangen @IanVS @valentinpalkovic