Skip to content
This repository was archived by the owner on Nov 27, 2018. It is now read-only.

Conversation

@delapuente
Copy link
Contributor

This PR removes the dependency with sw-precache at the same time it provides a basic servie worker.

@delapuente
Copy link
Contributor Author

@mykmelez or @marco-c , do you want to take a look?
I missed sw-precache is able to take a template parameter to generate. I'm preparing another PR not removing the sw-precache but with the custom template. What do you think?

@mykmelez
Copy link
Contributor

@mykmelez or @marco-c , do you want to take a look?

You bet! Looking at this now…

I missed sw-precache is able to take a template parameter to generate. I'm preparing another PR not removing the sw-precache but with the custom template. What do you think?

That's a good idea, since we're about to release v1, and using sw-precache with a custom template is a smaller set of changes, so it should be less risky. (We'll probably still remove sw-precache eventually, so I would keep this PR open and rebase it once we land that one.)

lib/offline.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: with this change, I'd also modify the sentence to read, "so the worker doesn't include itself in the list of files to cache."

@delapuente delapuente mentioned this pull request Nov 14, 2015
@delapuente
Copy link
Contributor Author

All done, @mykmelez do you mind to take a second look? I also changed how the worker prepares the new cache to be populated: before adding content, it deletes it in case the last installation failed and we would have a partially populated cache.

lib/offline.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: no need to define another function, simply do stream.on('finish', resolve);

@mykmelez
Copy link
Contributor

@delapuente Is this still a work-in-progress? It seems pretty ready, but the issue summary still has a WIP tag.

lib/offline.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you could avoid defining these two variables (data and hash) and directly do `hash: getHash(fs.readFileSync(filepath));', to make the function cleaner.

@marco-c
Copy link
Contributor

marco-c commented Nov 17, 2015

I did a second pass today, it looks good to me. I wish we had tests that actually executed the service worker code, but the changes look safe enough to land to me.

@delapuente
Copy link
Contributor Author

My onboarding session finished. I'm going to remove the WIP tag, fix all the nits and I will open a follow up for testing the service worker, how do you want to test the service worker? We can use karma-sw-mocha as a testing environment if you want. It's being used in ServiceWorkerWare.

@delapuente delapuente changed the title [WIP] Public domain sw Public domain sw Nov 19, 2015
@delapuente
Copy link
Contributor Author

@mykmelez for some reason, this is not working in the latest Nightly but it's doing perfectly in Chrome. I'm going to review the worker because I don't know what's happening.

@marco-c
Copy link
Contributor

marco-c commented Nov 19, 2015

We can use karma-sw-mocha as a testing environment if you want. It's being used in ServiceWorkerWare.

Sounds good. I guess you can set it up pretty easily since you wrote it 😄

@delapuente
Copy link
Contributor Author

Well, what does not work is controlled updating. The service worker got updated and installed but there is no updatefound event.

After some research, it turns out the sw-precache version is not working either. I think it could be related with bug 1189659. I have some test cases (very simple workers) where I got it to run in Nightly but I fail making the oghliner version to work. I will be talking to bkelly or baku to solve this mistery.

Anyway, I think the new service worker is ready enough. What do you think @marco-c @mykmelez ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still like to understand what this comment is suggesting, but I'm not going to block landing the branch on this.

@mykmelez
Copy link
Contributor

Anyway, I think the new service worker is ready enough. What do you think @marco-c @mykmelez ?

I agree, I'm going to merge it, and then we can tackle any issues in followups. Thanks for all the iterations!

mykmelez added a commit that referenced this pull request Nov 19, 2015
@mykmelez mykmelez merged commit 5b223c5 into mozilla:master Nov 19, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants