Skip to content

add API for response chunks#176

Merged
kratiahuja merged 5 commits intoember-fastboot:masterfrom
mainmatter:response-chunks
Nov 29, 2017
Merged

add API for response chunks#176
kratiahuja merged 5 commits intoember-fastboot:masterfrom
mainmatter:response-chunks

Conversation

@marcoow
Copy link
Contributor

@marcoow marcoow commented Nov 22, 2017

This adds a new method chunks to the Result class that returns the HTML in chunks:

  • if there are no shoeboxes in the document, it returns 2 chunks - one for the header and one for the body
  • if there are shoeboxes in the document, it returns n chunks - one for the header, one for the body (except the shoeboxes that are at the bottom of the body) and one for each shoebox

The idea behind this is that this could be used in e.g. https://github.com/ember-fastboot/fastboot-express-middleware to send a streaming response so that big shoeboxes don't delay the whole document.

closes #174

const HTMLSerializer = new SimpleDOM.HTMLSerializer(SimpleDOM.voidMap);

const SHOEBOX_TAG_PATTERN = '<script type="fastboot/shoebox"';
const HTML_HEAD_REGEX = /^([\s\S]*<\/head>)([\s\S]*)/;
Copy link
Contributor

Choose a reason for hiding this comment

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

@marcoow it does not support body attributes recently added in #171

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean - this RegExp does not make any assumptions on how the <body> looks and works perfectly fine with a tag like <body data-foo="my value">.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcoow ah sorry for confusion, my bad - it says head> and I was reading body> 🤦

@marcoow
Copy link
Contributor Author

marcoow commented Nov 24, 2017

@kratiahuja, @rwjblue: do you have a minute to review?

@kratiahuja
Copy link
Contributor

@marcoow: sorry I have been travelling. I can review it early next week if that isn’t late.

@marcoow
Copy link
Contributor Author

marcoow commented Nov 27, 2017

Thanks @kratiahuja! I'm sure your busy so whenever you find the time is great but we'd like to use this from a release version ;)

Copy link
Contributor

@kratiahuja kratiahuja left a comment

Choose a reason for hiding this comment

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

LGTM overall.

* @returns {Promise<Array<String>>} the application's DOM serialized to HTML, split into chunks
*/
chunks() {
return insertIntoIndexHTML(this._html, this._head, this._body, this._bodyAttributes).then((html) => {
Copy link
Contributor

@kratiahuja kratiahuja Nov 29, 2017

Choose a reason for hiding this comment

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

@marcoow A minor optimization that can be done here is:

Instead of replacing the entire index.html, you may want to think about only replacing the chunk lazily. This will enable sending the head faster. Can be done at a later point too. I think with streaming you want to make sure you do an early flush as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, ideally we could stream the head as soon as it is rendered, even while the rest of the document is still being rendered. My understanding is that that is currently not possible though as we have to wait until visit resolves at which point the complete document has been rendered already? Also I guess in order for that to work we'd have to make this a generator function rather than returning a promise?

Or maybe you're referring to something else that I don't get :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I wasn't refering to waiting for visit. But right now we parsing the entire index.html and replacing body as well when we only want to send the head first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point. But I think we'd have to turn this into a generator function then though to be able to give control back to the caller once the first chunk has been calculated. That would be a breaking change though…

@kratiahuja kratiahuja merged commit 9ea6d87 into ember-fastboot:master Nov 29, 2017
@kratiahuja
Copy link
Contributor

@marcoow I'll release a new version tonight.

@marcoow
Copy link
Contributor Author

marcoow commented Nov 29, 2017

@kratiahuja: awesome - thanks!

@marcoow marcoow deleted the response-chunks branch November 29, 2017 20:22
@kratiahuja
Copy link
Contributor

@marcoow fastboot@1.1.2 is released!

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.

Allow chunked transfer encoding

3 participants