add request cache, change Request to extend Body, fix Body, implemen…

[DomingosMartins]

add request cache
change Request to extend Body
fix Body
implement client.postmessage to receive a callback function

[zackargyle]

It looks like passing in a postMessageCallback is not part of the Client spec, and causes a backwards incompatibility if someone is passing in frameType currently. Can you provide more details of the reasoning for this?

[DomingosMartins]

I've passed this callback in order to pass a mocked function to properly test if post-message was called. I can pass is to a different parameter.

[zackargyle]

Cool. Can you share a snippet of what your test looks like? We might be able to find a better way than adding non-spec code to the mocks 🤞🏻

[DomingosMartins]

it('should update the clients', async done => {
    const defer = when.defer();
    const clientPostCallback = response => {
        defer.resolve(response);
    };
    const responseBody = { test: 'response' };

    responseMock = new Response(responseBody, { headers: headersMock });
    global.fetch = jest.fn(() => responseMock);
    await clients.openWindow(
        'http://localhost:3000',
        clientPostCallback
    );

    requestMock = new Request('http://localhost:3000/test', {
        cache: 'reload'
    });

    expect(global.fetch.mock.calls.length).toBe(0);
    const response = await self.trigger('fetch', requestMock);
    expect(global.fetch.mock.calls.length).toBe(1);
    expect(response).toEqual(responseMock);

    setTimeout(() => {
        defer.reject('Post message has not been called from the client');
    }, 2000);

    when(defer.promise).then(
        callbackResponse => {
            expect(callbackResponse.data).toEqual(responseBody);
            done();
        },
        failure => {
            done(failure);
        }
    );
});

[zackargyle]

Awesome. Can you point me to when in this example postMessage should be called?

[DomingosMartins]

This is the test, it will be called when we intercept the fetch.

[DomingosMartins]

const updateClients = async (event, response) => {
    const clients = await self.clients.matchAll();
    const data = await response.json();
    const { clientId } = event;
    clients.forEach(client => {
        if (clientId !== client.id) {
            client.postMessage({
                data
            });
        }
    });
};

const cachedResourceOrFetch = async event => {
    // some more logic
    const response = fetch('url');
    updateClients(event, response.clone());
    return response.clone();
};

self.addEventListener('fetch', async event => {
    event.respondWith(cachedResourceOrFetch(event));
});

[zackargyle]

I see. It's hard to suggest a better way to test it without the full example, but I think the simplest way would be to override the mock on the client itself. That way we're not adding non-spec code to the models themselves.

const client = await clients.openWindow('http://localhost:3000');
client.postMessage = jest.fn();

[zackargyle]

^ Does that work for you? This should make it easy to verify that postMessage is being called (but that's not really a great unit to test).

I'm trying to have the mocks act exactly like the spec, and adding extra parameters is undesirable. There's no good way to update the postMessage method itself since that talks to a separate DOM environment. The only better way would be to use puppeteer and write a unit test to verify that the postMessage is working correctly in your app.

[zackargyle]

This does not look like part of the spec.

[DomingosMartins]

Same as above.

[zackargyle]

Do we need the if statements? I think it should just be

constructor(body, init) {
  super(body);
  ...
}

[DomingosMartins]

When you clone the Response the body will be already a Blob

[zackargyle]

We disabled Travis on this repo, but somehow your diff got in a weird state with it. I'll be able to get it merged once we resolve comments!

[DomingosMartins]

Sorry,

I was out for some weeks, I've removed the implementation of the post message.
I thought that this was a good approach to test the postMessage as this is a lib only to unit test the service workers. :)

[zackargyle]

Can you explain this line? I believe it will break if body is a string.

[DomingosMartins]

JSON.stringify() does not break with a string

[zackargyle]

typeof 'foo' === 'string', so it won't get into that if statement, instead it will be set to undefined. That's why I was curious about the else if statement. What was the reasoning behind the fallback to undefined?

[sodabrew]

I've found my way to this PR as I'm writing up a test harness for my scripts on Cloudflare's Workers environment. Specifically I found that Response does not currently extend Body, which this PR adds.

Is there anything I can do to help the code review above to move this forward?

(Thank you for this project!)

[zackargyle]

Resolved in #92