Navigation Menu

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

feat(ssr): Render to stream #1197

Merged
merged 15 commits into from Jun 26, 2020
Merged

feat(ssr): Render to stream #1197

merged 15 commits into from Jun 26, 2020

Conversation

CyberAP
Copy link
Contributor

@CyberAP CyberAP commented May 17, 2020

Provide a renderToStream API for a quicker SSR rendering. It would allow to start streaming as soon as the first synchronous render happens, awaiting on all the next async renders upon encounter. All async operations still execute in parallel, so it works exactly like renderToString but streams as soon as possible, while renderToString always awaits for all the promises to return a result.
Right now it doesn't resolve teleports and I don't know if it even should.
All the tests are a direct copy of those of renderToString with renderToStream converted into Promise for simplicity.
You could notice there's a code duplication for helper functions, I thought I should leave it up to maintainers for restructuring.

@CyberAP CyberAP changed the title feat: render to stream feat(ssr): Render to stream May 17, 2020
Copy link
Member

@yyx990803 yyx990803 left a comment

Choose a reason for hiding this comment

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

Great work! I think the implementation looks good.

Obviously we should try to reduce the redundancy by reusing some of the duplicated logic - I can do that but it will have to wait until I finish other priorities. Would you be interested in working on that in the meanwhile?

stream: Readable
): Promise<void> {
try {
for await (const item of buffer) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to only await when the item is a Promise - this would save a microtask tick on sync buffer entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fine now I think

@CyberAP
Copy link
Contributor Author

CyberAP commented May 18, 2020

Great work! I think the implementation looks good.

Obviously we should try to reduce the redundancy by reusing some of the duplicated logic - I can do that but it will have to wait until I finish other priorities. Would you be interested in working on that in the meanwhile?

Sure, I'll mention you when it's ready.

@CyberAP CyberAP requested a review from yyx990803 May 23, 2020 14:12
@CyberAP
Copy link
Contributor Author

CyberAP commented May 24, 2020

@yyx990803 there's one problem remaining still that I'm not sure which way is exactly the best to tackle it.

SSR compiler would produce optimized code only for renderToString, but not for renderToStream, since runtime helpers exist only for the former. I thought of two ways dealing with it: a compiler setting or a runtime check somehow. Maybe there's a better solution?

@yyx990803
Copy link
Member

@CyberAP I'm not quite sure what you mean by

since runtime helpers exist only for the former

Do you have an example?

@CyberAP
Copy link
Contributor Author

CyberAP commented Jun 12, 2020

@yyx990803 there are two kinds of buffer right now: one for renderToString and another for renderToStream. The methods that renderers expose (renderComponentSubtree for example) are bound to buffer constructor and only renderToString methods are exposed. That means when runtime helpers are executed they will use renderToString buffer and will not be as effective if we are rendering to stream. So instead of an array the buffer will return a promise if any async setup is encountered and we'll have to wait for everything to be resolved first.

Another solution I can think of is to always use a buffer that returns an array and await on deep promises at unroll stage, the same way it's done in renderToStream.

@yyx990803
Copy link
Member

always use a buffer that returns an array and await on deep promises at unroll stage, the same way it's done in renderToStream.

I think that is better - if both use the same buffer implementation then there's also no need to create bound versions of the render methods.

@CyberAP
Copy link
Contributor Author

CyberAP commented Jun 12, 2020

Great, I'll be able to refactor that on Monday then.

@yyx990803 yyx990803 merged commit 6bc0e0a into vuejs:master Jun 26, 2020
@yyx990803
Copy link
Member

Great job!

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.

None yet

2 participants