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

fix(v2): restore prefetch functionality #3723

Merged
merged 8 commits into from
Nov 16, 2020
Merged

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 11, 2020

Motivation

PR #3001 apparently broke prefetching of chunk assets as hash was added to route name. In this PR I tried to fix it by finding needed route chunks by its key (without hash). I suppose that if similar logic worked earlier, then it is acceptable now. I just don't see any reason to include the crypto-js library and generate route md5 hash (if this is possible inside the client bundle, then performance will probably suffer and the bundle size will increase, so this filtering by key is probably the only alternative option.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See Network panel (#1375)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Nov 11, 2020
@lex111 lex111 requested a review from slorber as a code owner November 11, 2020 07:39
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 11, 2020
@netlify
Copy link

netlify bot commented Nov 11, 2020

Deploy preview for docusaurus-2 ready!

Built with commit ae6e8fb

https://deploy-preview-3723--docusaurus-2.netlify.app

@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

Thanks, didn't notice that the prefetching was broken, as it seems preloading still works.

I think this solution is already an improvement, but what about this case:

  "/docs/2.0.0-alpha.65-6d2": {
    "component": "component---theme-doc-page-1-be-9be",
    "versionMetadata": "versionMetadata---docs-2-0-0-alpha-656-db-dcc"
  },
  "/docs/2.0.0-alpha.65/-a63": {
    "component": "component---theme-doc-item-178-a40",
    "content": "content---docs-2-0-0-alpha-65-e-91-019"
  },

Imagine route to prefetch is /docs/2.0.0-alpha.65-6d2, in such case, I think we should prefetch both routes, not just the first one we find.

What about:

  • normalizing the route and chunkName regarding the potential trailing slash (I mean, to: "/docs/version or to: "/docs/version/ should not affect the prefetching)
  • removing the -xyz hash suffix? this way we can do an exact match, without computing the route hash again
  • prefetching all the matching chunks, not just the first one that matches?

@lex111
Copy link
Contributor Author

lex111 commented Nov 12, 2020

normalizing the route and chunkName regarding the potential trailing slash (I mean, to: "/docs/version or to: "/docs/version/ should not affect the prefetching)

Yes, it sounds good, but I still don't understand which of the routes we need to prefetch (with versionMetadata or content)?

removing the -xyz hash suffix? this way we can do an exact match, without computing the route hash again

But then we will lose the result of work from #3001?

prefetching all the matching chunks, not just the first one that matches?

Perhaps this is even the best solution, because we need both of them chunks?

@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

Yes, it sounds good, but I still don't understand which of the routes we need to prefetch (with versionMetadata or content)?

We should prefetch both. One is for the docs version layout (including version metatadata), the other is for the version home doc.

We need all this data anyway to render /docs/

But then we will lose the result of work from #3001?

Not exactly, the goal is not to revert this PR.

The problem with this issue is that:

  "/docs/2.0.0-alpha.65/": {
    "component": "component---theme-doc-page-1-be-9be",
    "versionMetadata": "versionMetadata---docs-2-0-0-alpha-656-db-dcc"
  },
  "/docs/2.0.0-alpha.65/": {
    "component": "component---theme-doc-item-178-a40",
    "content": "content---docs-2-0-0-alpha-65-e-91-019"
  },

Was merged into

  "/docs/2.0.0-alpha.65/": {
    "versionMetadata": "versionMetadata---docs-2-0-0-alpha-656-db-dcc"
    "component": "component---theme-doc-item-178-a40",
    "content": "content---docs-2-0-0-alpha-65-e-91-019"
  },

The first component attribute was overridden by the 2nd.

IE, we loose the information that we actually need the component---theme-doc-page-1-be-9be chunk.

What we want instead is:

  "/docs/2.0.0-alpha.65/-abc": {
    "component": "component---theme-doc-page-1-be-9be",
    "versionMetadata": "versionMetadata---docs-2-0-0-alpha-656-db-dcc"
  },
  "/docs/2.0.0-alpha.65/-def": {
    "component": "component---theme-doc-item-178-a40",
    "content": "content---docs-2-0-0-alpha-65-e-91-019"
  },

And if the user visit /docs/2.0.0-alpha.65/, then we'll fetch the 4 chunks.

I'm thinking of something like this instead:

const  routesChunkNamesMatch = Object.entries(routesChunkNames).filter((r) =>
        removeHash(r[0]) === match.route.path
)) 

@lex111
Copy link
Contributor Author

lex111 commented Nov 12, 2020

I'm thinking of something like this instead:

Or just use filter instead of find in current PR to get all needed chunks?

@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

yes, filter instead of find, as long as you get the 2 entries back instead of just one it should be fine

@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

but the problem is that if you are on / and filter for entries using starsWith("/"), you'll end up prefetching all chunks of the site instead of the ones we want to prefetch?

@lex111
Copy link
Contributor Author

lex111 commented Nov 13, 2020

@slorber I refactored initial code to get all needed chunk names, can you please check out this?

@slorber
Copy link
Collaborator

slorber commented Nov 16, 2020

the logic works fine

Refactored/documented/improved TS before merge so that I remember what's going on in the future

@github-actions
Copy link

github-actions bot commented Nov 16, 2020

Size Change: +133 B (0%)

Total Size: 152 kB

ℹ️ View Unchanged
Filename Size Change
website/build/blog/2017/12/14/introducing-docusaurus/index.html 20.7 kB +42 B (0%)
website/build/docs/introduction/index.html 180 B 0 B
website/build/index.html 5.84 kB +38 B (0%)
website/build/main.********.js 108 kB +62 B (0%)
website/build/styles.********.css 17.4 kB -9 B (0%)

compressed-size-action

@slorber slorber merged commit 3ab7336 into master Nov 16, 2020
@slorber slorber changed the title fix(v2): attempt to restore prefetch functionality fix(v2): restore prefetch functionality Nov 16, 2020
@lex111 lex111 deleted the lex111/prefetch-restore branch November 17, 2020 15:01
@lex111 lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants