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: Improve webpack chunk names #16513

Merged
merged 3 commits into from
Nov 2, 2021
Merged

Core: Improve webpack chunk names #16513

merged 3 commits into from
Nov 2, 2021

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Oct 28, 2021

Issue: N/A

UPDATE: Removed the prefetching and kept the nice chunk names. See rationale and planned followup below

What I did

With the new story store and stories being lazy loaded, the initial loading experience is great but the experience when switching stories is degraded. This PR adds two things:

1 - Webpack chunk names.
Important to identify what's actually being downloaded rather than random names like:

and have more meaningful names like:

2 - Prefetched chunks

Here's a video comparison of the user experience without and with prefetching enabled in a Slow 3G setting.

Before (without prefetching):

without-prefetch.mov

After (with prefetching):

with-prefetch.mov

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Oct 28, 2021

Nx Cloud Report

CI ran the following commands for commit 9170830. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@yannbf
Copy link
Member Author

yannbf commented Oct 28, 2021

Hey @tmeasday @shilman I opened this PR to generate a discussion. I don't know if the approach from this PR will prefetch every single story there is, but ideally the prefetch should be done only for the visible stories in the viewport, or when hovering the element in the sidebar.

@shilman
Copy link
Member

shilman commented Oct 28, 2021

This is awesome @yannbf

@MichaelArestad
Copy link
Contributor

YES! Prefetch all the things! I plan to preload the font files as well. This is great!

@tmeasday
Copy link
Member

tmeasday commented Oct 29, 2021

Interesting @yannbf! I would prefer to be more intentional about which stories we prefetch, as you say, but perhaps prefetching everything is better than prefetching nothing (in the meantime)?

We could also put a feature flag on it, but that might be getting a bit fiddly. WDYT @shilman?

@shilman
Copy link
Member

shilman commented Oct 29, 2021

What does it do to startup time?

@tmeasday
Copy link
Member

What does it do to startup time?

Ideally nothing right?, the aim of the prefetch annotation is to fetch the resources once the "site" is ready. Would be interesting to know if this is actually the case, for sure!

@yannbf
Copy link
Member Author

yannbf commented Oct 29, 2021

What does it do to startup time?

Regarding prefetch it should not do anything. What it will do is append link prefetch tags at runtime like so:

And that should also not impact the initial user experience. The scripts will be fetched and cached for later use only at idle time and at low priority for the browser. The only downside of this approach is that the user will be prefetching all kinds of stories that they might never click, but the good thing is that those assets will be cached for later use if they do use. I think it's a good first iteration, then we might do some smart strategy for prefetching only a portion of stories based on some heuristics.

Now, for the chunk name part, I believe it does change things in build time, but from changing the execution of a dynamically generated hash to the chunk name based on the file name.. it might be negligible? I tested building with and without the flag and the times were pretty much the same (I didn't do a clinical check)

@yannbf yannbf force-pushed the feature/story-prefetch branch from 5c8a673 to 9e06963 Compare October 29, 2021 07:11
@tmeasday
Copy link
Member

tmeasday commented Nov 1, 2021

And that should also not impact the initial user experience. The scripts will be fetched and cached for later use only at idle time and at low priority for the browser.

@yannbf did you test it out?

I just tried it in Chromatic's Storybook and I am seeing a definitely slowdown in the time to render the first story (from around 1 -> 2.5s in my rough metrics) . I'm not sure why the browser isn't waiting to fetch the files but it seems to launch of a massive batch of requests straight away.

I would be leaning towards maybe not including it given that, unless we can find some way to make the browser deprioritize it.

@shilman
Copy link
Member

shilman commented Nov 1, 2021

we'll follow up in 6.5 with a custom approach that eagerly loads all links that are visible AFTER the entire storybook has loaded, to avoid the slowdown mentioned above.

thanks for the awesome POC @yannbf !!! 💯

@shilman shilman closed this Nov 1, 2021
@shilman shilman self-assigned this Nov 1, 2021
@shilman shilman reopened this Nov 1, 2021
@shilman
Copy link
Member

shilman commented Nov 1, 2021

Closing this with #16546 as a followup

@shilman shilman closed this Nov 1, 2021
@tmeasday
Copy link
Member

tmeasday commented Nov 1, 2021

@shilman I think we may as well keep the nicer bundle/chunk name part of this?

@shilman shilman reopened this Nov 2, 2021
@shilman shilman changed the title Core: prefetch stories on idle browser time Core: Improve webpack chunk names Nov 2, 2021
@shilman shilman added this to the 6.4 PRs milestone Nov 2, 2021
@shilman
Copy link
Member

shilman commented Nov 2, 2021

merging the modified PR without prefetching

@shilman shilman merged commit 613aa0f into next Nov 2, 2021
@shilman shilman deleted the feature/story-prefetch branch November 2, 2021 03:41
@shilman shilman added maintenance User-facing maintenance tasks and removed feature request labels Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder-webpack5 core maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants