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

♻️ Refactor to gather story snapshot dom without reloading the page #592

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

wwilsman
Copy link
Contributor

@wwilsman wwilsman commented Jun 25, 2022

What is this?

When relying on the snapshot method to capture the DOM, each story is loaded from scratch. For some larger Storybooks, this can create a lot of extra time repeatedly initializing all of Storybook's JavaScript for each story.

By refactoring to gather DOM from a single page, this initial load time only happens once, and asset discovery can be performed on the static DOM snapshot without needing JavaScript enabled. In practice, this seems to speed up most Storybooks by roughly 20-40% depending on how well optimized the stories are. For poorly optimized stories, we also automatically handle page crashes and will only error if a page crashes on the same story more than once.

When testing large Storybooks of 800+ stories, it takes about 2 minutes (on my machine). However if that is too slow for some (or if stories are slow to load), a few additional flags were added to split snapshots into shards to allow parallel processes for the same Percy build. The new flags --shard-count or --shard-size will determine how to split snapshots, and --shard-index will determine which shard to use for the current process. Also the --partial flag from the exec command was incorporated, which can mark a build as partial to hide missing snapshots in the UI.

Also, I didn't realize this wasn't a feature already, but globals can now be provided the same as story args.

TODO

  • I have some slight changes to CLI that need to make it upstream so a util we need is exported:
    ✨ Export core server util cli#976
  • I found that we never enabled test coverage in CI after the refactor to CLI. That should be added to the GH action.
  • I also found that coverage was not actually working because babel cannot inject the code into ES modules. This is fixed by using a minimal node loader to properly transform ES modules with babel.
  • I also also found that because we never enabled test coverage in CI after the refactor, that we are missing quite a bit of coverage. We should add some tests to get coverage up (including new tests for the new flags).

The remaining todos feel out of scope for this PR since it relates to an existing issue our repo has had since the first CLI refactor. We'll address those in a follow up PR so we can push this major feature over the line.

@wwilsman wwilsman added the ✨ enhancement New feature or request label Jun 25, 2022
@wwilsman wwilsman requested a review from Robdel12 June 25, 2022 01:04
@wwilsman wwilsman force-pushed the ww/quick-dom-capture branch from 26fc55a to eb800fd Compare June 25, 2022 01:06
When relying on the snapshot method to capture the DOM, each story is loaded from scratch. For some
larger Storybooks, this can create a lot of extra time repeatedly initializing all of Storybook's
JavaScript for each story.

By refactoring to gather DOM from a single page, this initial load time only happens once, and asset
discovery can be performed on the static DOM snapshot without needing JavaScript enabled. In
practice, this seems to speed up most Storybooks by roughly 20-40% depending on how well optimized
the stories are. For poorly optimized stories, we also automatically handle page crashes and will
only error if a page crashes on the same story more than once.

When testing large Storybooks of 800+ stories, it takes about 2 minutes (on my machine). However if
that is too slow for some (or if stories are slow to load), a few additional flags were added to
split snapshots into shards to allow parallel processes for the same Percy build. The new flags
`--shard-count` or `--shard-size` will determine how to split snapshots, and `--shard-index` will
determine which shard to use for the current process. Also the `--partial` flag from the exec
command was incorporated, which can mark a build as partial to hide missing snapshots in the UI.
@wwilsman wwilsman force-pushed the ww/quick-dom-capture branch from eb800fd to ebd5970 Compare June 27, 2022 18:06
@wwilsman wwilsman marked this pull request as ready for review June 27, 2022 18:55
"cross-spawn": "^7.0.3",
"qs": "^6.11.0"
"@percy/cli-command": "^1.4.0",
"@storybook/router": "^6.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I was hopeful to kill this dep, but it came back when we had to rebuild the URL right? 😭

Copy link
Contributor Author

@wwilsman wwilsman Jun 27, 2022

Choose a reason for hiding this comment

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

Unfortunately, yes. While we can set story args and globals before capturing the DOM, it seems Storybook does not actually update the URL from internal state changes (besides when the story ID changes).

...percy.config.storybook,
baseUrl: url
}, () => {
log.debug(`Page crashed while loading story: ${stories[0].id}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be helpful for debugging poison stories 💯


return waitFor(async () => {
// use newer storybook APIs before old APIs
await (window.__STORYBOOK_PREVIEW__?.extract?.() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with on demand now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trick seems to be you need to call extract before calling raw now. Looking at Storybook's own code for extracting stories.json, they do something similar (but with puppeteer).

Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁

@wwilsman wwilsman merged commit 9feeba3 into master Jun 27, 2022
@wwilsman wwilsman deleted the ww/quick-dom-capture branch June 27, 2022 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants