Skip to content
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

sw-cache not working (for offline use) #30

Closed
wzulfikar opened this issue May 21, 2017 · 30 comments
Closed

sw-cache not working (for offline use) #30

wzulfikar opened this issue May 21, 2017 · 30 comments
Assignees

Comments

@wzulfikar
Copy link

Is it intentional that sw cache doesn't work out of the box?

I did preact create my-great-app, build the project using npm run build and serve build/ using php simple server.

I visit my localhost, the site is up, I can see the code to register service worker in /bundle.js but the registration code wasn't executed. However, it works if i manually register it again, i.e. by executing navigator.serviceWorker.register('/sw.js', { scope: '/' }); in browser console.

I'm using Opera 45 in OSX.

@prateekbh prateekbh self-assigned this May 21, 2017
@prateekbh
Copy link
Member

@developit AFAIK we deliberately stopped sw from localhost, should we document it why?
Also should we enable sw on localhost?

@lukeed
Copy link
Member

lukeed commented May 22, 2017

I believe @wzulfikar is using a standard http: server, based on "php simple server". If that's the case, then there definitely won't be any offline caching happening. This is the relevant hook condition.

Again, @wzulfikar, if this is the case, you should try running npm run serve instead of your PHP server. This will fire up a HTTPS server, which will enable the offline caching.

@NekR
Copy link

NekR commented May 22, 2017

@lukeed Then manual navigator.serviceWorker.register('/sw.js', { scope: '/' }); won't work, but it works. Sounds more like what @prateekbh described.

@lukeed
Copy link
Member

lukeed commented May 22, 2017

@NekR I'm not sure what you're saying? If you manually type in navigator.serviceWorker.register(...), you've bypassed the if() condition that the bundle includes. So, of course it's going to register...?

@NekR
Copy link

NekR commented May 22, 2017

@lukeed you just said you believe @wzulfikar is using http: and ServiceWorker cannot be registered on http: in whatever situation.

@prateekbh
Copy link
Member

@lukeed i have a question... even if its a simple http server service workers are built with an exception of localhost(for the https condition). I still believe that we should allow sw in localhost.

@NekR
Copy link

NekR commented May 22, 2017

IMO, this check is too weak: https://github.com/developit/preact-cli/blob/222e7018dd360e40f7db622191aeca62d6ef0c9a/src/lib/entry.js#L7, i.e. as said here it doesn't allow localhost because it localhost can register ServiceWorker even on http: (this is an exception to my previous comment).

What you need is this check: https://github.com/NekR/offline-plugin/blob/master/tpls/runtime-template.js#L8

(who could thought about that plugin 😏)

@prateekbh
Copy link
Member

^agreed to @NekR

@lukeed
Copy link
Member

lukeed commented May 22, 2017

@NekR Yes you can -- localhost is a built-in exception. I don't know if that's a cross-browser behavior, but it definitely works in Chrome.

This is why a location.protocol === 'https' check is important. Otherwise you get developers asking why their assets/development site no longer updates.

You can use my preact-starter, build the production files, then go in & remove the https check & you'll see it working @ http://localhost:3000. I do it all the time -- it's how I test Service Workers (on demand only).

Or see below:

screen shot 2017-05-22 at 1 29 05 am

@NekR
Copy link

NekR commented May 22, 2017

@NekR Yes you can -- localhost is a built-in exception. I don't know if that's a cross-browser behavior, but it definitely works in Chrome.

I don't really know why you answer it to me like I was asking if localhost works. I'm telling you it works and it works in every browser which supports ServiceWorker.

@NekR
Copy link

NekR commented May 22, 2017

This is why a location.protocol === 'https' check is important. Otherwise you get developers asking why their assets/development site no longer updates.

Should they test SW on production or have pre-production server with https:?

@lukeed
Copy link
Member

lukeed commented May 22, 2017

@NekR I was replying to

ServiceWorker cannot be registered on http: in whatever situation.

Let's not take this off-topic. I agree with the current checks in place, but can re-discuss this elsewhere. But FWIW, what you've added is both redundant (the hostname check) and incomplete (there is also 0.0.0.0 and 10.0.0.x in addition to 127...).

Once @wzulfikar has a chance to respond, we can figure out if this issue is closed or not. If, in fact, a standard HTTP server was used, then this works as expected & should be closed.

@prateekbh
Copy link
Member

prateekbh commented May 22, 2017

@lukeed the localhost check is cross browser. localhost is exempted from the https check for serviceworker

@lukeed
Copy link
Member

lukeed commented May 22, 2017

Should they test SW on production or have pre-production server with https:?

If you run npm run serve instead of a custom PHP server (as is the case here), you do get a HTTPS server.


Edited wrong command name.

@prateekbh
Copy link
Member

If you run npm start instead of a custom PHP server (as is the case here), you do get a HTTPS server.

@lukeed just curious wouldn't that need dev to configure the certificate on his machine?

@lukeed
Copy link
Member

lukeed commented May 22, 2017

@prateekbh No, it's done & included for you. Check it out! 🎉

@lukeed
Copy link
Member

lukeed commented May 22, 2017

It also includes H2 push. 🙌

screen shot 2017-05-22 at 1 40 46 am

@NekR
Copy link

NekR commented May 22, 2017

If you run npm start instead of a custom PHP server (as is the case here), you do get a HTTPS server.

But then project won't be updating during development as you just said in previous comments. What's the different with allowing it on localhost in whatever situation?

Let's not take this off-topic.

What's off-topic in my comments? But that http2 comment is off-topic, so maybe open another issue for it?

But FWIW, what you've added is both redundant (the hostname check) and incomplete (there is also 0.0.0.0 and 10.0.0.x in addition to 127...).

Users of offline-plugin which use it for ears and was asking for specific checks very much disagree with you. Also for FYI, 0.0.0.0 and 10.0.0.x require https: and aren't treated as localhost or 127.*.*.*.
image

But for sure, redundant 👍

@lukeed
Copy link
Member

lukeed commented May 22, 2017

Redundant because SW is already internally checking if location.hostname === 'localhost'.

And, the primary requirement for SW is HTTPS. As pointed out, SW has its own built-in secure contexts and its own built-in exceptions. This is why the current condition is sufficient.

Off-topic because this issue is about @wzulfikar's usage specifically. Let's save this entire debate about the (in)adequacy of the checks for a separate discussion. It does not pertain directly to the question asked.

I'm happy to discuss this further 😀 I just don't think this is the place for it. That's all 👍

@prateekbh
Copy link
Member

And, the primary requirement for SW is HTTPS. As pointed out, SW has its own built-in secure contexts and its own built-in exceptions. This is why the current condition is sufficient.

@lukeed dont you feel an additional if(!localhost) is an interference from our side to the built-in exceptions?

@lukeed
Copy link
Member

lukeed commented May 22, 2017

I'm not advocating for !localhost. I'm supporting !http.

There's no need to cache and test-run http://localhost... especially with HMR active.

There is a need and/or reason to test-run https://localhost... and that's what is already in place.

@NekR
Copy link

NekR commented May 22, 2017

Redundant because SW is already internally checking if location.hostname === 'localhost'.

I have my reasons, but by following your logic https: check is also redundant.

And, the primary requirement for SW is HTTPS. As pointed out, SW has its own built-in secure contexts and its own built-in exceptions. This is why the current condition is sufficient.

Double standards. What security you bring by banishing localhost & co?

Off-topic because this issue is about @wzulfikar's usage specifically. Let's save this entire debate about the (in)adequacy of the checks for a separate discussion. It does not pertain directly to the question asked.

Adding correct checks directly solves @wzulfikar's issue. Not everybody uses node.js. What a surprise.

@NekR
Copy link

NekR commented May 22, 2017

@lukeed are you fighting for "my beautiful software" here? If something, this particular tool is written for people (yeah), not only for...

@developit
Copy link
Member

I didn't realize we were manually exempting localhost - I've never had to do that before and never had issues with HMR. Maybe I've just been using devtools' bypass feature though?

@wzulfikar
Copy link
Author

wzulfikar commented May 22, 2017

@prateekbh that's the catch. I didn't know if we deliberately stop sw from localhost because I used sw before this and what I knew is, sw can work only in https OR in localhost. Thus, when I tried it in localhost and it didn't work, I thought there's something wrong.

Now that I know we deliberately stop sw from localhost, I can confirm the offline feature works out of the box, checked it by tunnelling my local php -S to public using ngrok and access my local server using ngrok's https.

Using npm run serve will setup localhost with https out of the box (as told by @lukeed). I can access the site using https but the call to sw failed.

screen shot 2017-05-22 at 10 54 51 pm

Probably, we can just put in the readme about the state of offline feature in localhost so people don't bother testing it and for them to not getting confused when sw doesn't work in localhost.

@prateekbh
Copy link
Member

@lukeed @developit first thing i'll do is to add this note in README. But i guess this will surely get a debate going on about it, as devs AFAIK do test sw on normal localhost as well

@wzulfikar
Copy link
Author

thanks @prateekbh :) Shall we close this issue for now?

@prateekbh
Copy link
Member

I'll open a parallel issue and close this

@wzulfikar
Copy link
Author

noted :)

@developit
Copy link
Member

Just a note here - I think we need to include some instructions around accepting the self-signed SSL cert somewhere. People are tripping over it, but it's fairly important. Really nice once you get the cert added and you can have a green lock during development :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants