Adds workbox.streams #1439

jeffposnick · 2018-04-19T20:33:32Z

Fixes #621

I'll add some inline comments with anything that I'd imagine could be contentious/questionable to aid in the review.

jeffposnick · 2018-04-19T20:35:36Z

infra/testing/activate-sw-safari.js

@@ -3,15 +3,17 @@ module.exports = async (swUrl) => {
  let error = await global.__workbox.webdriver.executeAsyncScript((swUrl, cb) => {
    const onStateChangePromise = (registration, desiredState) => {
      return new Promise((resolve, reject) => {
-        if (registration.installing === null) {
-          throw new Error('Regstration is not installing');
+        if (desiredState === 'activated' && registration.active) {


I made some tweaks to this logic because I've seen some flakiness with the latest Safari TP, where registration.installing was null and this function gave up.

I think it's less prone to race conditions with these changes, since if we have a registration.active and the state we care about is 'activated', that should be fine.

jeffposnick · 2018-04-19T20:36:58Z

infra/testing/sw-env-mocks/Headers.js

@@ -18,7 +18,7 @@
 // https://fetch.spec.whatwg.org/#headers-class
 class Headers {
  constructor(obj = {}) {
-    this.obj = obj;
+    this.obj = Object.assign({}, obj);


This works around an issue that I encountered in the tests.

https://github.com/pinterest/service-workers/blob/master/packages/service-worker-mock/models/Headers.js looks like it's much more robust than it used to be, and perhaps we should migrate off of our own mock at some point in the future.

jeffposnick · 2018-04-19T20:38:30Z

packages/workbox-streams/_default.mjs

+  };
+}
+
+export default {


I'm (still) not 100% sure what belongs in the _default.mjs's named exports and what doesn't.

I think this leads to a decent public interface, and I can use workbox.streams.strategy(), workbox.streams.isSupported(), etc. with this setup. But please double-check my thinking.

jeffposnick · 2018-04-19T20:39:20Z

packages/workbox-streams/_public.mjs

+
+import './_version.mjs';
+
+export {


Similar to my previous comment about uncertainty re: what should be in the _default.mjs export, I'm not 100% sure here, either.

jeffposnick · 2018-04-19T20:40:28Z

packages/workbox-streams/concatenate.mjs

+    fullyStreamedReject = reject;
+  });
+
+  if (process.env.NODE_ENV !== 'production') {


This series of log messages looks like the following in .dev.:

Could you change this to a single group:

Concatenating 5 sources.

And instead of ...reading..., Reached the end of that source under each group, could you just add success or failure along with the final response.

While I understand the desire to log the requests as the state changes, it doesn't work with console groups as everything gets out of sync and ends up accidentally nesting and mixing - instead you need to capture the logs and then log everything at once.

jeffposnick · 2018-04-19T20:43:45Z

test/workbox-streams/integration/test.js

@@ -0,0 +1,41 @@
+const {expect} = require('chai');


Creating meaningful node-based mocks for all the primitives needed by workbox.streams wasn't feasible, so I'm relying on the integration tests to exercise most of the actual logic.

gauntface · 2018-04-19T23:09:15Z

demos/functions/views/demo/workbox-streams/sw.hbs

+  () => `<p>this was executed at ${Date.now()}</p>`,
+  ({event}) => httpBinStrategy.makeRequest({
+    event,
+    request: 'https://httpbin.org/bytes/12',


don't use a third party.

gauntface

Core logic looks good. Largely minor comments on the log and demo.

gauntface · 2018-04-19T23:10:55Z

demos/functions/views/demo/workbox-streams/sw.hbs

+  event.waitUntil((async () => {
+    const cache = await caches.open(CACHE_NAME);
+    await Promise.all([
+      cache.put(START_CACHE_KEY, new Response('<html><head></head><body>')),


Would be preferable to make this a little more "fun".

Perhaps you could have an iframe that displays a HTML page indicating its not streamed and with SW installed it loads a HTML page with a header and footer with a party popper in the middle. This highlights a clear use case of header and footer and party popper highlights its working.

gauntface · 2018-04-19T23:12:11Z

packages/workbox-streams/_default.mjs

+ * `'text/html'` will be used by default.
+ * @return {workbox.routing.Route~handlerCallback}
+ *
+ * @memberof workbox.streams


Does this work if you run gulp docs?

I think you need to define the @module workbox.streams before you assign memberof's

I think it works as expected? Here's what the gulp docs output looks like:

It's not styled, obviously, but neither are the other modules' docs. And that appears to be the correct info.

gauntface · 2018-04-19T23:15:37Z

packages/workbox-streams/_default.mjs

+  concatenate,
+  concatenateToResponse,
+  isSupported,
+  strategy,


I'm not sure strategy is the best name here. workbox.streams.join() workbox.streams.run() workbox.streams.merge() would explain the intent a little easier.

join() and merge() are effectively synonyms of the existing concatenate(), and I feel like using those names would confuse things and not make it clear what's different about strategy().

The thinking behind strategy() is that this is an interface that matches the interfaces exposed under workbox.strategies.*. So if a developer is familiar with one of the existing strategy interfaces, they'd presumably get that workbox.streams.strategy() is doing the same thing.

If you'd really prefer something other than strategy(), I guess I'd be okay with runt(), but handle() would be my first alternative choice. Or we could stick with strategy() 😄

I'm not sure this having the same API means it should have the same name unless it is behaving the same.

gauntface · 2018-04-19T23:16:51Z

packages/workbox-streams/concatenate.mjs

+  // TODO: This should be possible to do by constructing a ReadableStream, but
+  // I can't get it to work. As a hack, construct a new Response, and use the
+  // reader associated with its body.
+  return new Response(source).body.getReader();


cc @jaffathecake in case he has any ideas

gauntface · 2018-04-19T23:22:45Z

packages/workbox-streams/concatenate.mjs

+    fullyStreamedReject = reject;
+  });
+
+  if (process.env.NODE_ENV !== 'production') {


Could you change this to a single group:

Concatenating 5 sources.

And instead of ...reading..., Reached the end of that source under each group, could you just add success or failure along with the final response.

While I understand the desire to log the requests as the state changes, it doesn't work with console groups as everything gets out of sync and ends up accidentally nesting and mixing - instead you need to capture the logs and then log everything at once.

gauntface · 2018-04-19T23:23:38Z

packages/workbox-streams/concatenate.mjs

+            logStartOfGroup = true;
+            if (process.env.NODE_ENV !== 'production') {
+              logger.log('Reached the end of that source.');
+              logger.groupEnd();


Dynamic log groups like this will cause issues - we had with some of the previous strategies. Instead you need to stash the logs you want to make and then log everything together.

Thanks for the heads-up about nesting and groups. I've refactored that.

I don't have access to the new Response at this point in the code, only to the ReadableStream. That doesn't end up exposing any useful information when it's logged to the console.

I'm going to take advantage of the fact that the Response will normally be logged by folks who use our Router class, and consider that sufficient.

It now looks like:

coveralls · 2018-04-23T18:44:07Z

Coverage decreased (-1.8%) to 87.296% when pulling 5de7d21 on streams-take-two into d3e3a88 on master.

gauntface

Would be good to get @philipwalton's +2

philipwalton · 2018-04-26T18:36:03Z

packages/workbox-streams/concatenate.mjs

+    },
+  });
+
+  return {done, stream};


I remember we had a discussion before about whether done was really needed for waitUntil or whether respondWith was sufficient to keep the SW alive. Did we ever confirm that one way or another?

I confirmed with Jake that while the relevant specifications are pointing towards this no longer being required in the future, the current implementation in Chrome (and likely other browsers) does require explicitly calling waitUntil() to guarantee the SW is kept alive until the stream is drained.

Ok, cool. Thanks for confirming.

philipwalton · 2018-04-26T19:24:52Z

LGTM

jeffposnick added 6 commits April 18, 2018 15:42

Fresh checkin/fresh branch.

92b7e97

Tests.

7086193

Linting.

4904628

Hopefully reduce some flakiness in the Safari activateSW logic.

69701d2

Logging.

d5f2783

Demo.

bb8fca2

jeffposnick requested review from addyosmani and gauntface April 19, 2018 20:33

jeffposnick commented Apr 19, 2018

View reviewed changes

Fixed a logger import

3e6d1d1

gauntface reviewed Apr 19, 2018

View reviewed changes

gauntface suggested changes Apr 19, 2018

View reviewed changes

Review feedback.

5de7d21

gauntface approved these changes Apr 24, 2018

View reviewed changes

jeffposnick requested a review from philipwalton April 24, 2018 16:41

philipwalton reviewed Apr 26, 2018

View reviewed changes

jeffposnick merged commit 45cb8d0 into master Apr 26, 2018

jeffposnick deleted the streams-take-two branch April 26, 2018 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds workbox.streams #1439

Adds workbox.streams #1439

jeffposnick commented Apr 19, 2018

jeffposnick Apr 19, 2018

jeffposnick Apr 19, 2018

jeffposnick Apr 19, 2018

jeffposnick Apr 19, 2018

jeffposnick Apr 19, 2018

gauntface Apr 19, 2018

jeffposnick Apr 19, 2018

gauntface Apr 19, 2018

jeffposnick Apr 23, 2018

gauntface left a comment

gauntface Apr 19, 2018

jeffposnick Apr 23, 2018

gauntface Apr 19, 2018

jeffposnick Apr 23, 2018

gauntface Apr 19, 2018

jeffposnick Apr 23, 2018

gauntface Apr 24, 2018

gauntface Apr 19, 2018

gauntface Apr 19, 2018

gauntface Apr 19, 2018

jeffposnick Apr 23, 2018

jeffposnick Apr 23, 2018

coveralls commented Apr 23, 2018

gauntface left a comment

philipwalton Apr 26, 2018

jeffposnick Apr 26, 2018

philipwalton Apr 26, 2018

philipwalton commented Apr 26, 2018


		import './_version.mjs';

		export {

Adds workbox.streams #1439

Adds workbox.streams #1439

Conversation

jeffposnick commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 23, 2018

gauntface left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipwalton commented Apr 26, 2018