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

Enable id option in compiler for SSR #4856

Closed
wants to merge 7 commits into from

Conversation

AlbertMarashi
Copy link

@AlbertMarashi AlbertMarashi commented May 18, 2020

Issue: #4854

Feature

Allows giving compiler option for hash of a component's unique ID for SSR preloading of async/code-split scripts, so it can be matched to a client-manifest

I am planning for svelte-loader (hopefully) to pass a hash of the component's file path in SSR mode, so it can be matched to a user-provided client-manifest file plugin, enabling a server-side renderer to provide preload/async/prefetch scripts for faster application loading speeds

Why is this important

A server-side renderer has no way to know which async imports may have been loaded, and cannot know which client scripts & files to http2/push or prefetch/preload on the client-side. This results in a water-fall effect, which can severely slow loading speeds for SSR applications:
image

This is an essential feature to support for enterprise applications that require async routing and SSR and will increase loading speeds for apps that require asynchronous/dynamic imports

This method has been proven to work effectively, as VueJS uses a similar implementation.
https://ssr.vuejs.org/guide/bundle-renderer.html#enter-bundlerenderer

I have implemented an ideal scenario that results in 100/100 lighthouse score for apps with SSR, this is not currently possible with SSR apps that use async components

Context: https://twitter.com/PromatiaGov/status/1262183956004220928

AlbertMarashi added a commit to AlbertMarashi/svelte-loader that referenced this pull request May 18, 2020
AlbertMarashi added a commit to AlbertMarashi/svelte-loader that referenced this pull request May 18, 2020
if there is a svelte v10, this won't break. This also grabs us the minor version so we can use it in the if statement once PR sveltejs/svelte#4856 is merged and released
@AlbertMarashi
Copy link
Author

@Rich-Harris do you think this could be approved so that I can add the integration to svelte-loader?

src/compiler/compile/index.ts Outdated Show resolved Hide resolved
src/compiler/compile/render_ssr/index.ts Outdated Show resolved Hide resolved
src/runtime/internal/ssr.ts Outdated Show resolved Hide resolved
@pngwn
Copy link
Member

pngwn commented May 19, 2020

Don't @ people to push things forward, we can all see it but there are many pull requests. It will be dealt with in due course.

This branch is still failing CI regardless.

@AlbertMarashi AlbertMarashi changed the title Enable hash option in compiler for SSR Enable id option in compiler for SSR May 19, 2020
@antony
Copy link
Member

antony commented May 19, 2020

@DominusVilicus I'm not exactly sure what you're doing here but suggesting changes to your own PR and then merging them is an incredibly slow and unreliable feedback loop for development, not to mention spamming discord.

Might I suggest that you make these changes on your local environment and then just push to your branch?

Unless I'm missing something...

@AlbertMarashi
Copy link
Author

AlbertMarashi commented May 19, 2020

@antony my bad, I will do that in the future.

I'm not exactly sure what you're doing here

If you are talking about the purpose of the pull request, it is to enable an option to the compiler so that the server-side renderer can know exactly which components have been registered, so that it can be matched against a client-manifest to perform asset injection on the client-side, to prevent waterfall requests like this, which can dramatically slow down client-responsiveness

image

Asset injection ensures that the necessary scripts are prefetched/preloaded for the client in parallel (and enables the possibility of http/2 PUSH for async output files)

image

This is currently not possible, and if you think it is, I would need to see how you would implement this.

EDIT
This will be (hopefully) used by the svelte-loader (WIP) sveltejs/svelte-loader#124

@Rich-Harris
Copy link
Member

Thanks for the PR. How would you feel about using options.filename in place of adding a new id field? Reasons: a) it's already used by bundler plugins, b) is guaranteed to be unique, and c) each component gets it for free if it's being compiled via a bundler.

I think we could rename registeredComponents to renderedComponents (I would suggest just 'components', but I can imagine wanting to reserve that for something in future, e.g. something that facilitates some sort of post-SSR optimisation). Also, I think it might be more natural if we convert the Set to an array before exposing it.

@AlbertMarashi
Copy link
Author

@Rich-Harris that might be a better idea, I will see what I can do. renderedComponents seems good components can be reserved.

renderedComponents suits better for server-side-rendering anyways

@AlbertMarashi
Copy link
Author

AlbertMarashi commented May 21, 2020

Lovely, am able to preload/prefetch necessary scripts with this feature

image

@Rich-Harris because this is an option that doesn't change the output/expected html, I'm not sure how to add a test for it, since it's not like most feature tests.

Something like this might be possible

const { js } = //get test component, and apply custom filename option: compile(string, { filename: 'test' }
let Component = //turn JS into component
const { renderedComponents} = Component.render();

if(renderedComponents[0] !== 'test'){
 //fail test
}

@AlbertMarashi
Copy link
Author

Could we merge this & close #4854? @Rich-Harris

@domingues
Copy link

domingues commented Jun 28, 2020

I solved this problem on my SSR example (https://github.com/domingues/svelte-ssr-example) with a simple rollup plugin: https://www.npmjs.com/package/rollup-plugin-extract-bundle-tree

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Aug 24, 2020

@domingues that doesn't solve the problem. I can get the exported files easily, the concern is how to know which files were executed during SSR so that they can be http2 pushed to the client.

Your solution only works on the route-level, but I want it to work with deeply nested asynchronous components too.

@AlbertMarashi
Copy link
Author

Hey @pngwn @antony & @Rich-Harris. I hate to ping again, but could we get this merged, it's been a couple months and this feature would be pretty essential for websites that want faster SSR & client speeds.

Removing the waterfall issue will improve the speed and responsiveness of Svelte apps that are using deeply nested asynchronous components.

@allofthenamesaretaken
Copy link

I also need this feature, as it stands I cannot currently preload nested async components

@benmccann
Copy link
Member

benmccann commented Sep 5, 2020

Thanks for this! And sorry it's taken you awhile to get it in. I just implemented preloading in Sapper. This seems like it could help improve that implementation

One thing I wonder about using options.filename is that it then embeds the full working directory that you were in when you compiled the component into the compiled output. That makes it such that the builds are not reproducible - if you build on different machines you are likely to get different output. I think it could be a good idea to put the filename relative to the cwd instead.

Also, you'll need to add renderedComponents to the docs here: https://github.com/sveltejs/svelte/blob/master/site/content/docs/03-run-time.md#server-side-component-api

This could use a test as well

@benmccann
Copy link
Member

benmccann commented Sep 5, 2020

I'm trying to think about how you then map the renderedComponents to the bundled JS chunk that needs to be loaded. We should test whether you can do that in a component that you get from npm

Another thing I realized is that this will only work for hydrated content. It doesn't help you figure out what to preload in the case that you're rendering only on the client side, so you might need two preload implementations: one for ssr on and one for ssr off

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Sep 6, 2020

@benmccann thank you for your feedback.

That's completely right, you will need to be using SSR to know what components were actually rendered so that they can be preloaded. That's why the renderedComponents is only exposed in SSR.

One thing I wonder about using options.filename is that it then embeds the full working directory that you were in when you compiled the component into the compiled output. That makes it such that the builds are not reproducible - if you build on different machines you are likely to get different output. I think it could be a good idea to put the filename relative to the cwd instead.

The filename is determined by svelte-rollup-plugin, so if we were going to make the filename relative to the cwd it would need to be done there.

You can always remove the absolute part of the URL and make it relative to cwd just by getting the filenames from the renderedComponents, so I don't consider it much of an issue.

I'd imagine that most people would build their apps on each deployment anyways.


@benmccann
Copy link
Member

I sent a PR to rollup-plugin-svelte to make the path relative. Assuming that gets merged, this PR looks good to me.

@DominusVilicus would you mind adding a test to close this out?

@AlbertMarashi
Copy link
Author

@benmccann I would love to but I am not sure how to add a test for this specific PR, it's a bit different from the rest. Could you perhaps guide me or potentially create one yourself?

@benmccann
Copy link
Member

I haven't worked on Svelte at all before, so am probably not the best person to give guidance on testing this. I mainly work on Sapper

@AlbertMarashi
Copy link
Author

Closed in favour of #5476

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

Successfully merging this pull request may close these issues.

8 participants