fixed registration problem of service worker #1807
Conversation
|
|
|
@harshkhandeparkar , @jywarren Could you please review this |
| }); | ||
| }); | ||
|
|
||
| const installingWorker = registration.installing; |
There was a problem hiding this comment.
this is happening bcoz we have already checked service worker state as installed and now this state installing will not come again. so it returns a null on which we have just added a statechange event listener which is meaningless.
There was a problem hiding this comment.
I see, thank you!; this is so hard to track as maintainers; do you think it's possible to write a test for this?
There was a problem hiding this comment.
I have tried to write test for this but i don't know how to make this run 😅 ?
you can have a look this test -
https://github.com/vivek-30/image-sequencer/blob/5fc7fc8ecbb02ac989605a4d422b7b8d85d67aeb/test/core/sw.test.js#L1-L72
@jywarren, @harshkhandeparkar could you please suggest me what to do?
There was a problem hiding this comment.
Actually @jywarren what i think is even if we don't build a test for this then also it will solve this problem.
No doubt that adding test will ensure more security and safety but don't think it will slow down image sequencer as to check a successful installation of sw .we have to remove existing sw and clear out the cache and then register a new updated sw (which i have done in my test) ?
There was a problem hiding this comment.
Thanks for giving it a try! If you add the test to the correct folder alongside others in this repo, it'll run within this PR. It's possible you need to add some more assertions -- like, see how this line confirms something we expect to be true about the test scenario:
Want to try adding your test in this PR?
There was a problem hiding this comment.
@jywarren plz have a look now ,i have added a test for this. Hope you like it
test/core/sequencer/sw.js
Outdated
| var sw = require('../../examples/sw'); | ||
| var test = require('tape'); | ||
|
|
||
| function SWInstallation(registration){ |
There was a problem hiding this comment.
Here, should we instead use the setupCache() method from our actual code, rather than make a unique worker management code segment within the tests? Does that make sense? What do you think? Then our assertions below ought to still pass, right?
There was a problem hiding this comment.
sure sir ,it will avoid code duplication.
And i also thinks it will still pass.
There was a problem hiding this comment.
@jywarren could you please review these changes
|
Thanks for sticking with this one @vivek-30 this was a complex one to fix and test and we appreciate it!!!! |
|
Thanks a lot sir 😊 . you really helped me to solve the issue. |
Fixes #1138.