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

UI: Enable TurboSnap for UI Storybook #19767

Merged
merged 14 commits into from
Nov 8, 2022
Merged

Conversation

ethriel3695
Copy link
Contributor

Issue:

Storybook is using up quite a bit of its story allotment within Chromatic.
We use Vite to build Storybook, which does not work out of the box with the Chromatic feature TurboSnap

Add a package to allow the Storybook monorepo to use TurboSnap

What I did

  • Added the package vite-plugin-turbosnap
  • Updated the main.ts Storybook config file to include the package

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@ethriel3695 ethriel3695 added the build Internal-facing build tooling & test updates label Nov 7, 2022
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.

Thanks Reuben!

We need to change the CircleCI config though, as it doesn't contain any Git history. From the CLI output:

Retrieving git information
⚠ TurboSnap disabled due to missing git history
Could not retrieve changed files since baseline commit(s).
This typically happens after rebasing, force pushing, or when running against an ephemeral merge commit.
ℹ Read more at https://www.chromatic.com/docs/turbosnap#how-it-works

Maybe you can reach out to Chromatic Support and get this resolved? 🤣

code/tsconfig.json Outdated Show resolved Hide resolved
@JReinhold JReinhold self-assigned this Nov 7, 2022
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.

.

@JReinhold JReinhold changed the title Reuben/add turbosnap for UI UI: Enable TurboSnap for UI Storybook Nov 7, 2022
@ethriel3695
Copy link
Contributor Author

Thanks Reuben!

We need to change the CircleCI config though, as it doesn't contain any Git history. From the CLI output:

Retrieving git information
⚠ TurboSnap disabled due to missing git history
Could not retrieve changed files since baseline commit(s).
This typically happens after rebasing, force pushing, or when running against an ephemeral merge commit.
ℹ Read more at https://www.chromatic.com/docs/turbosnap#how-it-works

Maybe you can reach out to Chromatic Support and get this resolved? 🤣

I've updated the depth to 5 for the git-shallow-clone orb that we use for git checkout
https://circleci.com/developer/orbs/orb/guitarrapc/git-shallow-clone

If that does not work, we may want to consider altering this job to utilize the CIrcleCI helper checkout instead.

@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.

😵‍💫 Bin script confusion

This package has multiple bin scripts with the same name. This can cause non-deterministic behavior when installing or could be a sign of a supply chain attack

Consider removing one of the conflicting packages. Packages should only export bin scripts with their name

Package Bin script Location
jest@29.2.2 (added) jest test-storybooks/external-docs/package.json via @storybook/react-webpack5@7.0.0-alpha.43, @storybook/preset-react-webpack@7.0.0-alpha.43, jest-specific-snapshot@4.0.0,test-storybooks/standalone-preview/package.json via @storybook/react-webpack5@7.0.0-alpha.43, @storybook/preset-react-webpack@7.0.0-alpha.43, jest-specific-snapshot@4.0.0
jest-cli@29.2.2 (added) jest test-storybooks/external-docs/package.json via @storybook/react-webpack5@7.0.0-alpha.43, @storybook/preset-react-webpack@7.0.0-alpha.43, jest-specific-snapshot@4.0.0, jest@29.2.2,test-storybooks/standalone-preview/package.json via @storybook/react-webpack5@7.0.0-alpha.43, @storybook/preset-react-webpack@7.0.0-alpha.43, jest-specific-snapshot@4.0.0, jest@29.2.2
Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ⚠️ 2 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 ✅ 0 issues
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 jest@29.2.2
  • @SocketSecurity ignore jest-cli@29.2.2

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev

@ethriel3695
Copy link
Contributor Author

ethriel3695 commented Nov 8, 2022

@JReinhold @tmeasday
It's interesting that the CI process indicates that TurboSnap was disabled because we could not find the git history
https://app.circleci.com/pipelines/github/storybookjs/storybook/31499/workflows/d5673531-0632-4f9a-b824-32c448749599/jobs/437615

Yet, the build in Chromatic clearly indicates that TurboSnap was successful and skipped snapshots as intended.
Screen Shot 2022-11-07 at 6 11 52 PM

I ran Chromatic locally and it appears that TurboSnap is working correctly.

Publish complete in 3 minutes 15 seconds
    → View your Storybook at https://635781f3500dd2c49e189caf-cgzyzceemp.chromatic.com
Verifying your Storybook
    → This may take a few minutes
Starting partial build
    → Snapshots will be limited to 0 story files affected by recent changes
✔ TurboSnap enabled
Capturing 0 snapshots and skipping 380 snapshots.
Started build 373
    → View build details at https://www.chromatic.com/build?appId=635781f3500dd2c49e189caf&number=373
✔ Storybook published
We found 72 components with 383 stories.
ℹ View your Storybook at https://635781f3500dd2c49e189caf-cgzyzceemp.chromatic.com

The change runs correctly on storybook:ui however, not for the chromatic-sandboxes job

I've modified the chromatic.ts task and added --only-changed to see if this might resolve the issue

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This is good, with a few tweaks.

It's interesting that the CI process indicates that TurboSnap was disabled because we could not find the git history

I think the message here is a little confusing, the issue is that snapshotting is entirely disabled for the storybook-blocks SB right now: https://www.chromatic.com/manage?appId=63591cc9eaf4e8caec2bb5e9

Yet, the build in Chromatic clearly indicates that TurboSnap was successful and skipped snapshots as intended.

You're mixing the two SBs that are tested in that command up, that's the storybook-ui SB which is working fine it seems.

The change runs correctly on storybook:ui however, not for the chromatic-sandboxes job

I've modified the chromatic.ts task and added --only-changed to see if this might resolve the issue

This is a bit trickier, as we would have to inject the vite config into each sandbox. It can be done, but we are looking to heavily reduce the number of sandboxes we run soon anyway. Otherwise we can take a task to do this on the SB team next cycle.

scripts/tasks/chromatic.ts Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@JReinhold
Copy link
Contributor

Yeah sorry for the confusion @ethriel3695
Just to make it clear, we have 3 types of sandboxes running:

  1. The UI Storybook - storybook:ui:chromatic, which contains all the manager and block components. This is the one we want TurboSnap for

  2. The Blocks Storybook - storybook:blocks:chromatic. It is just a subset of the UI Storybook that is for public consumption, and we've disabled UI Tests and UI Reviews for this, it's only for publishing purposes - so TurboSnap is irrelevant. Maybe this is what the CLI is trying to tell us, but with a weird error message?

  3. Sandboxes. @tmeasday I don't think we can ever use TurboSnap for these, because they are not checked into git, so there will never be a git history for them? Even if we checked in their webpack stats file I assume that wouldn't be enough, because TS still looks at changes to package.json, yarn.lock, etc.

  4. and 2. both run in the same CI job outside of the sandboxing tasks.

@tmeasday
Copy link
Member

tmeasday commented Nov 8, 2022

@JReinhold you might want to change the storybook:blocks:chromatic script to not pass --only-changed then. Or we can just live with the warning 🤷

Sandboxes. @tmeasday I don't think we can ever use TurboSnap for these, because they are not checked into git, so there will never be a git history for them? Even if we checked in their webpack stats file I assume that wouldn't be enough, because TS still looks at changes to package.json, yarn.lock, etc.

Well, hmm. They are in the same git repo. So they will have git changes. But because of the way they are built I don't think TS will work, as all the file changes will be in node_modules. So certainly, I don't think we should use TS for them.

@ethriel3695 as I mentioned we are reducing the number of sandboxes we run by 2-3x currently, FYI.

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.

Changes look good! 💪
I don't know why CI is failing though, some weird Chromatic CLI error.
Could be because you updated the Chromatic version?

Could you try merging next in and see if that fixed it?

@IanVS
Copy link
Member

IanVS commented Nov 8, 2022

Let me know if vite-plugin-turbosnap is ever misbehaving (I wrote it).

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

🙌

@tmeasday tmeasday merged commit 6e927f3 into next Nov 8, 2022
@tmeasday tmeasday deleted the reuben/add-turbosnap-for-ui branch November 8, 2022 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants