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

feat(storybook): bump to v8 #12957

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

TylerAPfledderer
Copy link
Contributor

Description

Now that the project uses NextJS v14, this PR bumps Storybook to v8.

  • Introduces the Visual Testing Addon to check snapshots locally before committing.
  • Creates a chromatic config for the above addon.
  • Adds a package subscript build-storybook:chromatic specifically for Chromatic. This runs storybook build in test mode to cut down the build time of storybook by stripping down the build to only what's needed for Chromatic
  • Adds build-storybook.log to .gitignore as local builds with the Visual Test Addon will generate this log, which should not be committed.
  • Replaces @storybook/testing-library with @storybook/test per compatibility. (This change currently exists in feat: setup Storybook Interaction Testing #12444)

@github-actions github-actions bot added the dependencies 📦 Changes related to project dependencies label May 14, 2024
Copy link

netlify bot commented May 14, 2024

Deploy Preview for ethereumorg failed. Why did it fail? →

Name Link
🔨 Latest commit 6f7a35d
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/664cbcb78777670008f12294

@pettinarip
Copy link
Member

@TylerAPfledderer weird error I see on the logs about sharp...I've triggered a new fresh build clearing cache just in case. https://app.netlify.com/sites/ethereumorg/deploys/6644e5a3feaff946f4f01742

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented May 15, 2024

@TylerAPfledderer weird error I see on the logs about sharp...I've triggered a new fresh build clearing cache just in case. https://app.netlify.com/sites/ethereumorg/deploys/6644e5a3feaff946f4f01742

@pettinarip Thanks! I have a couple of PRs that had unexpected build errors.

Looks like a new error here.

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented May 17, 2024

@pettinarip @corwintines leaving a note here:

Need to check the Visual Testing Addon to make sure it's going to function properly in other user locals, because of the generated projectId. The documentation for the Chromatic CLI states that this is also referred to as the appId but fails in my local when I actually use that id.

View the docs for Visual Testing Addon.

@pettinarip
Copy link
Member

@TylerAPfledderer tested locally and it didn't work with the set projectId. I've changed it to ours 63b7ea99632763723c7f4d6b and it worked. Build example https://www.chromatic.com/build?appId=63b7ea99632763723c7f4d6b&number=2406

.gitignore Outdated

# Storybook
build-storybook.log
Copy link
Member

Choose a reason for hiding this comment

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

We might also need to ignore /storybook-static since it is generated when you run the tests locally.

@@ -68,6 +69,8 @@ const config: StorybookConfig = {
return !(isStyledSystemProp || isHTMLElementProp)
},
},

reactDocgen: "react-docgen-typescript"
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this dep installed. Is it missing or is it a transitive dep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it's transitive? I'm not familiar with the term.

This is a part of the main storybook. It needs to be set in v8 because it was the default in v7, but they switched to react-docgen as the default. The one set here is needed to filter out all those extraneous Chakra props.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, ok. Yes, it is installed by sb 👍🏼

@TylerAPfledderer
Copy link
Contributor Author

@TylerAPfledderer tested locally and it didn't work with the set projectId. I've changed it to ours 63b7ea99632763723c7f4d6b and it worked. Build example https://www.chromatic.com/build?appId=63b7ea99632763723c7f4d6b&number=2406

@pettinarip Ok! Thank you for checking. I'll try to set it again in my local and see if anything changes, and clarify the errors I see.

To clarify, I think this visual testing addon is only useful for those who have access to the project in Chromatic in approving changes, but still might be helpful in its convenience for review.

@pettinarip
Copy link
Member

@TylerAPfledderer tested locally and it didn't work with the set projectId. I've changed it to ours 63b7ea99632763723c7f4d6b and it worked. Build example https://www.chromatic.com/build?appId=63b7ea99632763723c7f4d6b&number=2406

@pettinarip Ok! Thank you for checking. I'll try to set it again in my local and see if anything changes, and clarify the errors I see.

To clarify, I think this visual testing addon is only useful for those who have access to the project in Chromatic in approving changes, but still might be helpful in its convenience for review.

Got it, good to know. Ok, I've updated the branch with dev to fix the issue we had on the build. Once that finish, I think we should be good to go.

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented May 21, 2024

@pettinarip Updating the projectId: When I go to take a snapshot in the addon, I see this console error:

Chromatic CLI v11.3.2
https://www.chromatic.com/docs/cli

Skipping update check
Authenticating with Chromatic
    → Connecting to https://index.chromatic.com
    → No Access

✖ Failed to authenticate

Error communicating with the Chromatic API. Check if your Chromatic client is up-to-date.
Service status updates are provided at https://status.chromatic.com

→ FORBIDDEN: No Access

If you need help, please chat with us at https://www.chromatic.com/docs/cli for the fastest response.
You can also email the team at support@chromatic.com if chat is not an option.

Please provide us with the above CLI output and the following info:
{
  "timestamp": "2024-05-21T13:35:13.244Z",
  "sessionId": "28c5271b-37af-488b-9a6e-e98cc885786a",
  "nodePlatform": "linux",
  "nodeVersion": "21.7.3",
  "packageName": "chromatic",
  "packageVersion": "11.3.2",
  "flags": {
    "interactive": false
  },
  "extraOptions": {
    "projectId": "Project:63b7ea99632763723c7f4d6b",
    "autoAcceptChanges": false,
    "exitOnceUploaded": false,
    "exitZeroOnChanges": true,
    "forceRebuild": true,
    "fromCI": false,
    "interactive": false,
    "isLocalBuild": true,
    "skip": false,
    "skipUpdateCheck": true,
    "experimental_abortSignal": {}
  },
  "configuration": {
    "configFile": "chromatic.config.json",
    "projectId": "Project:63b7ea99632763723c7f4d6b",
    "zip": true,
    "buildScriptName": "build-storybook:chromatic"
  },
  "isLocalBuild": true,
  "buildScript": "storybook build --test",
  "exitCode": 202,
  "exitCodeKey": "GRAPHQL_ERROR",
  "errorType": "GraphQLError",
  "errorMessage": "✖ Failed to authenticate"
}

And the config:

{
  "projectId": "Project:63b7ea99632763723c7f4d6b",
  "zip": true,
  "buildScriptName": "build-storybook:chromatic"
}

Am I missing a permission?

@pettinarip
Copy link
Member

@TylerAPfledderer Yeah, I think only people who have permissions to the project will be able to run those tests locally. I had to authorize it before I could run it.

@pettinarip
Copy link
Member

@TylerAPfledderer pushed a commit to regenerate the yarn.lock file to see if that fix the issue with sharp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Changes related to project dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants