Skip to content

PWA: API for shadow dom streaming#9495

Merged
dvoytenko merged 4 commits intoampproject:masterfrom
dvoytenko:pwa23
May 24, 2017
Merged

PWA: API for shadow dom streaming#9495
dvoytenko merged 4 commits intoampproject:masterfrom
dvoytenko:pwa23

Conversation

@dvoytenko
Copy link
Copy Markdown
Contributor

This PR supersedes #9428. It creates an API for shadow doc streaming via the StreamWriter interface. It's still marked as experimental until I fully measure impact. At that point, the old codepath will be re-implemented via the streaming API.

In the end, the set of changes is very modest, but lots of tests.

Closes #9491.
Related to #9490.

/cc @bjalford


// Fetch.
const url = this.resolveUrl_(path);
const url = this.resolveUrl_(path) + '?stream=1000';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you want to keep this the default? If yes, please comment on it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Using the streaming API:
```javascript
const shadowDoc = AMP.attachShadowDocAsStream(hostElement, url, options);
fetch(url).then(response => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The real example here is the one including the polyfill above, right?

Does it make sense to provide a helper for stream fetching?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Possibly. I'll consider for a follow up PR. This API, however, looks the best to me so far on the generic level.

let transferCount = 0;
while (inputBody.firstChild) {
transferCount++;
targetBody.appendChild(inputBody.firstChild);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't fully understand how this works. Does it only stream direct children of body? What if they aren't closed yet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct. That's a neat part of https://jakearchibald.com/2016/fun-hacks-faster-content/ and https://html.spec.whatwg.org/multipage/syntax.html#stack-of-open-elements. When you move an "open" node, the parser continues to stream into it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

WOW.

Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

I think we should iterate on this a bit, but lets get it in.

@pbakaus
Copy link
Copy Markdown
Contributor

pbakaus commented May 23, 2017

All of this is epic win.

Copy link
Copy Markdown
Contributor

@pbakaus pbakaus left a comment

Choose a reason for hiding this comment

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

LGTM.

// but, TMK, these are not supported anywhere yet.
const reader = response.body.getReader();
const decoder = new TextDecoder();
function readChunk() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like we can hoist this closure outside of readChucnk

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

xhr.onreadystatechange = null;
reject(new Error(`Unknown HTTP status ${xhr.status}`));
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/* LOADING */?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@dvoytenko dvoytenko merged commit 44fcae8 into ampproject:master May 24, 2017
@dvoytenko dvoytenko deleted the pwa23 branch May 24, 2017 05:25
@jakearchibald
Copy link
Copy Markdown

Out of curiosity, how is the impact of this PR being measured?

@dvoytenko
Copy link
Copy Markdown
Contributor Author

@jakearchibald This is pretty experimental at this time. But bunch of tests I ran showed a big incremental speed up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants