Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Preload CSS and JS dependencies #1269

Merged
merged 1 commit into from Aug 7, 2020
Merged

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Jun 13, 2020

Closes #1260
Closes #408
Closes #440
Closes #712
Closes #449
Closes #405
Closes #70

I made a small library to extract the dependency tree from the Rollup OutputChunk[]. If you're okay with it being added a dependency here then I can go ahead and publish it to npm. By having it as an external project other projects can use it as well. Also, it made it easier to test because I wrote a test that operates on a JSON dump of a Rollup compilation result. Importing JSON in a test was kind of tricky and I had more control over changing settings for tsconfig, mocha, etc. in an external project to make that work

I ran this with npm link in the realworld project with Chrome set to emulate a slow 3g connection and it makes a big difference. You can see the JS files for the current page being loaded in parallel in the network tab instead of sequentially.

This PR

Screenshot from 2020-06-12 22-12-50

Current Deployed Real World Sample

Screenshot from 2020-06-12 22-20-50

Copy link
Member

@antony antony left a comment

Choose a reason for hiding this comment

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

Looks good, just needs a little tidy

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
runtime/src/server/middleware/get_page_handler.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@benmccann benmccann force-pushed the preload branch 3 times, most recently from a1aa284 to aaf7d17 Compare June 23, 2020 17:24
@habibrosyad
Copy link
Contributor

Hoping that this will get merged soon 👍

@benmccann benmccann merged commit f23ff3d into sveltejs:master Aug 7, 2020
trmcnvn pushed a commit to metafy-gg/sapper that referenced this pull request Aug 15, 2020
@bgjehu
Copy link

bgjehu commented Aug 18, 2020

@benmccann does this preloading dependencies feature works for sapper export?

@benmccann
Copy link
Member Author

If you run the Sapper server, then preload will be added as a header. Since you can't easily control headers on exported pages, I believe that there's some code in export that tries to turn the headers into <link rel="preload"> tags in the exported pages. I haven't tested the export functionality. That was around before this PR which wouldn't affect it and I haven't used export myself, but yes it should work - just a little differently

@riccardolardi
Copy link
Contributor

Also interested in whether the preloading will work in sapper export. @benmccann any hint on this?

@benmccann
Copy link
Member Author

I answered the question with all information I have about it just above: #1269 (comment)

@benmccann
Copy link
Member Author

I think I know why it's not working for export: #919 (comment)

I don't use export, so I'm not going to have time to fix it myself, but I'll review a PR if anyone wants to send one

@riccardolardi
Copy link
Contributor

@benmccann is it just about adding that line eg changing it to modulepreload? I‘d love to contribute if I‘m able

@benmccann
Copy link
Member Author

You'd want to support both, so I'd imagine the fix looks like this:

				if (ref.rel === 'preload' || ref.rel === 'modulepreload') {
					body = (body as string).replace('</head>',
						`<link rel="${ref.rel}" as=${JSON.stringify(ref.as)} href=${JSON.stringify(ref.uri)}></head>`);
				}

The main thing here is testing out the fix and making sure it works

@benmccann
Copy link
Member Author

I'm not sure what you're suggesting. routify.dev is built with Routify and not Sapper, so I'm not really sure how it's related. For comparison, Sapper sites score very highly on Google's Page Speed score. E.g. https://realworld.svelte.dev/ which is built with Sapper scores in the 90s.

@cortopy
Copy link

cortopy commented Sep 16, 2020

@benmccann sorry, I posted in the wrong thread. I moved the comment but notifications must've gone through anyway. My apologies. You should have the other one too I believe

@riccardolardi
Copy link
Contributor

@benmccann I've submitted a PR #1539 when you find some time I'd be thankful if you could review. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants