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

Add --playwright & --cypress flags #882

Merged
merged 11 commits into from
Jan 24, 2024
Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jan 9, 2024

When set, build storybook by directly invoking the binary from @chromaui/test-archiver.

This is a POC, we will move this binary into a chromatic-playwright package soon.

TODO:

  • Add cypress flag too
  • Ensure the flag works in the action
  • Throw errors if incompatible flags are used
  • Tests

Some notes:

  • This won't work if you symlink the chromatic package into a test project (due to symlinking and resolution). Probably just use the canary to test it out ([email protected])

  • For this to work in it's current form, you need to add the following export to the @chromaui/archive-storybook package.json:exports field:

    "./package.json": {
      "require": "./package.json",
      "import": "./package.json"
    }

I did this manually inside node_modules of my test project for now.

For some reason I couldn't require.resolve('@chromaui/archive-storybook') directly. @ndelangen may know more about this.

  • We'll want to resolve things in chromatic-playwright really, so we'd be adding the export to that package once we setup the monorepo.

  • This calls node path/to/@chromaui/archive-storybook/dist/bin/build-archive-storybook.js -o ..., which then calls node path/to/@storybook/cli/dist/bin/storybook.js -c path/to/config. There might be a way to avoid the double node invocation. Then again, it's probably not a big deal.

📦 Published PR as canary version: 10.6.0--canary.882.7635273249.0

✨ Test out this PR locally via:

npm install [email protected]
# or 
yarn add [email protected]

@tmeasday tmeasday requested review from tevanoff and thafryer January 9, 2024 11:50
@ndelangen
Copy link
Member

You can simplify:

    "./package.json": {
      "require": "./package.json",
      "import": "./package.json"
    }

to:

    "./package.json": "./package.json"

@tmeasday
Copy link
Member Author

tmeasday commented Jan 9, 2024

@ndelangen do you know why I could require.resolve() @chromaui/archive-storybook/package.json but not @chromaui/archive-storybook, given there is a exports field already containing '.'?

https://github.com/chromaui/archive-storybook/blob/26a2269c17b10ecf693d74c296b8254b1ece807c/package.json#L14-L20

@ndelangen
Copy link
Member

No, it should resolve to the full require path from here:
https://github.com/chromaui/archive-storybook/blob/main/package.json#L16C8-L16C36

So the outcome would be:
/path/to/node_modules/@chromaui//archive-storybook/dist/index.js

What error did you get? "could not resolve"?

@tmeasday
Copy link
Member Author

Yes, the standard MODULE_NOT_FOUND that we check for in this PR.

@tmeasday tmeasday force-pushed the tom/prototype-playwright-flag branch from 313988f to 4729958 Compare January 13, 2024 11:36
@tmeasday tmeasday changed the title Add a --playwright flag Add --playwright & --cypress flags Jan 13, 2024
@tmeasday tmeasday marked this pull request as ready for review January 13, 2024 12:09
When set, build storybook by directly invoking the binary from `@chromaui/test-archiver`.

This is a POC, we will move this binary into a `chromatic-playwright` package soon.
@tevanoff tevanoff force-pushed the tom/prototype-playwright-flag branch from 2400af5 to f87673a Compare January 20, 2024 01:13
node-src/lib/getE2eBinPath.ts Show resolved Hide resolved
@tmeasday
Copy link
Member Author

@tevanoff are you good with me merging this?

@tmeasday tmeasday added release Auto: Create a `latest` release when merged minor Auto: Increment the minor version when merged labels Jan 23, 2024
package.json Outdated
Comment on lines 199 to 200
"chromatic-playwright": "^0.3.1",
"chromatic-cypress": "^0.3.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to cut 1.0.0 versions of these when we go GA, we might want to include that here.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, we can do that. It'll still be possible to test (with incorrect versions) against the current versions if we do that, but it'll warn in some package managers, and error in npm: https://www.notion.so/chromatic-ui/Peer-Dependency-Investigations-ae77792bae6e420bbe860916bec10cbe?pvs=4#176332d500194cdfacadf1efd927c815

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I misinterpreted, you mean to have both versions supported 👍

Copy link
Contributor

@tevanoff tevanoff left a comment

Choose a reason for hiding this comment

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

Left one suggestion on the peer deps, but otherwise looks good!

@tmeasday
Copy link
Member Author

QA-ed on my test app w/ [email protected] and works well. The only issue I found was that because I had no scripts at all in my package.json, so I made this change: 42d2d2b

@tmeasday tmeasday added this pull request to the merge queue Jan 24, 2024
Merged via the queue into main with commit c5178db Jan 24, 2024
19 checks passed
@tmeasday tmeasday deleted the tom/prototype-playwright-flag branch January 24, 2024 03:49
@ghengeveld
Copy link
Member

🚀 PR was released in v10.5.0 🚀

@ghengeveld ghengeveld added the released Verdict: This issue/pull request has been released label Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Auto: Increment the minor version when merged release Auto: Create a `latest` release when merged released Verdict: This issue/pull request has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants