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

PWA: API for shadow dom streaming #9495

Merged
merged 4 commits into from May 24, 2017

Conversation

Projects
None yet
7 participants
@dvoytenko
Collaborator

dvoytenko commented May 23, 2017

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

@dvoytenko dvoytenko requested review from pbakaus, cramforce and choumx May 23, 2017

@googlebot googlebot added the cla: yes label May 23, 2017

@@ -149,8 +151,15 @@ class Shell {
}
// Fetch.
const url = this.resolveUrl_(path);
const url = this.resolveUrl_(path) + '?stream=1000';

This comment has been minimized.

@cramforce

cramforce May 23, 2017

Member

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

This comment has been minimized.

@dvoytenko

dvoytenko May 23, 2017

Collaborator

Removed.

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

This comment has been minimized.

@cramforce

cramforce May 23, 2017

Member

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

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

This comment has been minimized.

@dvoytenko

dvoytenko May 23, 2017

Collaborator

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);

This comment has been minimized.

@cramforce

cramforce May 23, 2017

Member

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

This comment has been minimized.

@dvoytenko

dvoytenko May 23, 2017

Collaborator

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.

This comment has been minimized.

@cramforce

cramforce May 23, 2017

Member

WOW.

@cramforce

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

@pbakaus

This comment has been minimized.

Collaborator

pbakaus commented May 23, 2017

All of this is epic win.

@pbakaus

LGTM.

const reader = response.body.getReader();
const decoder = new TextDecoder();
function readChunk() {
return reader.read().then(chunk => {

This comment has been minimized.

@jridgewell

jridgewell May 23, 2017

Member

Looks like we can hoist this closure outside of readChucnk

This comment has been minimized.

@dvoytenko

dvoytenko May 24, 2017

Collaborator

Done

reject(new Error(`Unknown HTTP status ${xhr.status}`));
return;
}
if (xhr.readyState == /* COMPLETE */ 3 ||

This comment has been minimized.

@jridgewell

jridgewell May 23, 2017

Member

/* LOADING */?

This comment has been minimized.

@dvoytenko

dvoytenko May 24, 2017

Collaborator

Done

@dvoytenko dvoytenko merged commit 44fcae8 into ampproject:master May 24, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dvoytenko dvoytenko deleted the dvoytenko:pwa23 branch May 24, 2017

@jakearchibald

This comment has been minimized.

jakearchibald commented Jun 22, 2017

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

@dvoytenko

This comment has been minimized.

Collaborator

dvoytenko commented Jul 6, 2017

@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