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

Core: Batch the loading of CSF files for extract() etc #20055

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Dec 2, 2022

Issue: We've seen situations where extract() overwhelms browsers due to loading too many requests at once

(only replicated on lambda-based browsers running old version of Chrome)

What I did

Batch loadAllCsfFiles() so it's no opening too many resources on the browser at once. The browser will only open so many requests to any given host anyway.

How to test

Run extract. This should happen as part of the chromatic-sandboxes task.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM. What's an appropriate batch size though? Isn't 100 pretty high?

code/lib/preview-api/src/modules/store/StoryStore.ts Outdated Show resolved Hide resolved
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@MichaelArestad MichaelArestad assigned tmeasday and ndelangen and unassigned tmeasday Dec 5, 2022
@ndelangen
Copy link
Member

@tmeasday There seems to be some problem with this code according to chromatic:

Do you have an error in your `preview.js`? Check your Storybook's browser console for errors.
    at PreviewWeb.extract (https://635781f3500dd2c49e189caf-pxsfegimbt.capture.chromatic.com/sb-preview/runtime.mjs:84:4851)
    at __puppeteer_evaluation_script__:86:33
    → Failed to publish build

@tmeasday
Copy link
Member Author

tmeasday commented Dec 6, 2022

Hmm, so that seems to be a weird flake, like the SB wasn't initialized properly when extract() was run. It's very odd given I was touching code inside extract, but following the code path, my changes wouldn't have influenced that error (i.e. the error comes earlier in the code path than loadAllCsfFiles().

Still it seems pretty suspicious. I ran the CI job again and it worked. I'll run it a couple more times to be sure...

@tmeasday tmeasday merged commit bafc895 into next Dec 6, 2022
@tmeasday tmeasday deleted the tom/sb-1013-batch-imports-in-loadallcsffiles-to branch December 6, 2022 05:59
@tmeasday
Copy link
Member Author

tmeasday commented Dec 6, 2022

Ok, reran it about half a dozen times without error. If anyone sees the above error again, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants