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: Add story preloading to optimize lazy compilation #17903

Merged
merged 13 commits into from Apr 8, 2022

Conversation

ndelangen
Copy link
Member

What I did

When using lazy-compilation, loading a story can take a bit of time, this tries to optimize that experience a bit, by guessing which story you'll visit next, and sending a command to the preview to preload the related stories ahead of time.

@nx-cloud
Copy link

nx-cloud bot commented Apr 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c01880d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen ndelangen self-assigned this Apr 6, 2022
@ndelangen ndelangen requested a review from tmeasday April 6, 2022 15:42
@shilman shilman changed the title add event for preloading stories on channel Core: Add story preloading to optimize lazy compilation Apr 7, 2022
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.

@ndelangen what's the best way to QA this?

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.

LGTM apart from doing unnecessary work.

Can we have a test in PreviewWeb that when it receives the event it loads the stories?

You can do something simple like:

const preview = await createAndRenderPreview();

importFn.mockReset();
await preview.onPreloadStory(['component-two--c'])

expect(importFn).toHaveBeenCalledWith(...);

lib/api/src/modules/stories.ts Outdated Show resolved Hide resolved
lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
@ndelangen ndelangen requested a review from tmeasday April 7, 2022 09:20
… is emitted

This only happens when StoryStoreV7 is enabled
@ndelangen
Copy link
Member Author

This can be tested in the react-ts example. @shilman and I tested it together 🎉

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.

Thanks @ndelangen! Couple nitpicks

lib/core-events/src/index.ts Outdated Show resolved Hide resolved
lib/preview-web/src/PreviewWeb.test.ts Outdated Show resolved Hide resolved
ndelangen and others added 3 commits April 8, 2022 12:05
Co-authored-by: Tom Coleman <tom@chromatic.com>
Co-authored-by: Tom Coleman <tom@chromatic.com>
@ndelangen ndelangen merged commit f975ec5 into next Apr 8, 2022
@ndelangen ndelangen deleted the feat/add-lazyload-event branch April 8, 2022 12:02
@yairEO
Copy link
Contributor

yairEO commented Apr 10, 2022

by guessing which story you'll visit next

Would have been nice to have included a short explanation of that logic

16 years ago I used to preload web pages when the mouse was hovering a link <a> more than N milliseconds threshold, so that's a good way of guessing where the user might click next

@ndelangen
Copy link
Member Author

@yairEO I'm preloading stories which would be navigated to if the user used the keyboard shortcuts to jump to the next and previous component.

@ndelangen
Copy link
Member Author

Great recommendation on the hover, would you be interested in implementing that as a contribution?

I could do it, but if it's something you'd be keen to do as well, I could assist! LMK

@@ -16,6 +16,8 @@ enum events {
FORCE_RE_RENDER = 'forceReRender',
// Force the current story to re-render from scratch, with its initial args
FORCE_REMOUNT = 'forceRemount',
// Request the story has been loaded into the store, ahead of time, before it's actually
Copy link
Contributor

Choose a reason for hiding this comment

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

The sentence ends abruptly :) before it's actually what?

Also, that is a missing word between story and has, like which

@yairEO
Copy link
Contributor

yairEO commented Apr 11, 2022

Great recommendation on the hover, would you be interested in implementing that as a contribution?

I could do it, but if it's something you'd be keen to do as well, I could assist! LMK

I would prefer you do it because it would take me a long long time to understand the codebase and I also don't know typescript... I try to avoid typescript until it dies like coffescript (hopefully!) :p

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.

None yet

4 participants