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

Extra check for registration service worker can be removed #6

Closed
aseem2625 opened this issue May 4, 2017 · 8 comments
Closed

Extra check for registration service worker can be removed #6

aseem2625 opened this issue May 4, 2017 · 8 comments

Comments

@aseem2625
Copy link

Hey @lukeed

This check location.protocol === 'https:' in src/index.js for service workers registration can be removed.

By default, localhost is treated safe and service workers works well.
And for production website, browsers by default won't allow service worker to work without https.

@lukeed
Copy link
Owner

lukeed commented May 4, 2017

Hey @aseem2625! I know, but it's still intentional. Assets will be cached in localhost, which is a major pain in the arse for development. Also, people who are still new to serviceWorker won't have any idea that this is happening. I've seen it happen a lot.

So, the https check is a quick way to negate this. Plus, even though SW won't register without https, it used to throw an error if you tried....in the early days, not so sure if it still would.

@lukeed lukeed closed this as completed May 4, 2017
@aseem2625
Copy link
Author

aseem2625 commented May 5, 2017

Thanks for the reply @lukeed

I'm unaware if SW used to throw error in http(Can't find if it still behaves like this). But otherwise, I feel that it shouldn't create any problem while development because it's added in "production" mode. And since npm run watch is much more frequently used while development than npm run build, npm start, developer would be aware that he is testing in production mode if he runs npm run build.

It will also further clarify in telling whether service worker is working fine.

Eg-
I removed this extra check and ran npm run build, npm start. And now loaded it in browser. First load works fine. Then I refreshed the page and SW also works fine as expected.

Now, I modified home.js (just removed few li items) and again ran npm run build, npm start and whoa, it shows the following error on simple page refresh unless I hard refresh my browser(chrome) to delete the SW cache:

image

Note: In this scenario, even if the page is hard refreshed once, subsequent simple refresh also throws the above error. I've to constantly hard refresh to load the pages. (Strange)

Ideally, webpack must be changing the hash of the files and the above error shouldn't have shown if service worker behaved perfectly fine. I wouldn't ever figure out this error if it's disabled for testing in localhost 😄

Other issue is that (happens only sometimes), when I load the page, it loads the files from service worker but instantly refreshes itself and loads all the files freshly. (Check this in Network tab in chrome dev tools)

So, testing of SW is important in localhost, otherwise developer will anyways have to disable the check to manually test.

What are your thoughts?

@dav-is
Copy link

dav-is commented Sep 19, 2017

@aseem2625 Seeing as this repository hasn't been updated in 5 months, maybe it's time you turn your fork into a full fledged repository so you can keep track of issues? Also, I noticed you haven't commited to your repo in around a month. If you need some help, I'd like for us to get this moving again. :)

Also, relating about this issue. I had a similar one in a different boilerplate repo where the not found component will be sent as a 200 instead of a 404 so the service worker tries to parse the html 404 page as javascript. You can confirm this by checking the network panel and seeing the response.

@aseem2625
Copy link
Author

aseem2625 commented Sep 19, 2017

@dav-is Yeh you're right. I've sent message to support for converting. I'll probably will resume working in a week.

Above issue was resolved long before. I was working on SSR. It works perfectly fine in build mode. 1) However, I'm yet to write configs (webpack) to make it run in development mode. It was a bit difficult for me, so it got delayed. 2) Another issue was that I wasn't finding the best way of sending the chunk specific style in .html from server. in isomorphic-style-loader which I used was adding duplicated style when JS kicks in at browser.

If you can help with any of the above 2, it would be great as I didn't find any best practices on net for these two.

@dav-is
Copy link

dav-is commented Sep 19, 2017

I’ll take a look at the development mode today and see what I can figure out. As for the styles, I’ve never used a dynamic css system before, but make sure it still supports CSP when it’s inplemented.

@lukeed
Copy link
Owner

lukeed commented Sep 19, 2017

Sorry for the radio silence. I'm 99% certain that this is a user-specific issue, sorry. I've used this starter, as is, across 4 different production projects, and they all worked well.

The same setup is active on my preact-starter kit, which is both more popular & more actively maintained. I've built dozens of Preact apps (in production) using the same setup. It works.

To reiterate, the HTTPS check on dev-mode is to prevent accidental caching during npm start, since that will be a production server, yet without an https origin. As mentioned earlier in this thread, I believe that's what you did during development & that's why you're having this issue.

Of course, if you don't like the HTTPS check, you can delete it. It's not a hidden or deeply-nested file. You can do whatever you please with it. But the HTTPS check is there to prevent these issues from being opened.

That said, you (both) are more than welcome to fork this repo & maintain your own copies. It's no secret that I haven't maintained this for a while. Truth be told, I haven't used Inferno in a long time.

If you're looking for any new boilerplate-type inspiration, check out that preact-starter, since that one was/is heavily related to this one and has been updated over time.

Thanks!

@dav-is
Copy link

dav-is commented Sep 19, 2017

I’m curious to your motivation to shifting away from inferno and towards Preact. I’m still not 100% sure which I want to use.

@lukeed
Copy link
Owner

lukeed commented Sep 19, 2017

I used Preact before Inferno. I've seen better application performance with Preact than Inferno. There's (sometimes) closer compatibility to React when in need of compat-layers -- and that shim-layer is also smaller for Preact. The total bytes down really does make a difference when targeting mobile users. And lastly, the development speed & process behind Preact is much more lively, active, and responsive than Inferno's.

I've learned a ton by working with & listening to the Inferno team. I was very happy & fortunate to be a part of their work. They still are doing amazing work, but there's no denying that it's slowed down a bit.

Either way, you're not worse off. They're both great with slightly different subtleties and use-cases. I used both simultaneously for a while and was able to switch from one project to another very seamlessly... no headfuckery.

The TL;DR is that I was using Preact first, its project goals are inline with my own goals, and I've found it to be more performant for my apps in the real world.

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

3 participants