-
Notifications
You must be signed in to change notification settings - Fork 829
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
fixing queue initialization bug #680
Conversation
@@ -37,7 +37,20 @@ describe('request-queue tests', () => { | |||
chai.assert.isObject(queue._config); | |||
}); | |||
|
|||
it('queueName is corrent', () =>{ | |||
it('initialize should re-fill the queue', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be misunderstanding the original problem, but this test doesn't actually test what happens when you call initQueue when there are no entries to be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I can cover that. However earlier on reboot of sw the queue was not getting re-initialized, which this should cover now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please add a test case for when this._idbQDb.get()
resolves to a null value, as it will the first time the SW starts up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test case doesn't actually cover the original problem.
On a slight tangent, I think part of the reason this bug came about in the first place is you need to set However, I think this is an anti-pattern because we don't know when the initialization will complete and it could lead to race conditions if you invoke another prototype method before completion. I think a better approach would be return the promise returned by |
+1 @philipwalton's comments here. You have a race condition that the external libraries can't detect or handle. Would be good to ensure initQueue is called before other methods are used as well. |
9e3b769
to
73d2f3c
Compare
I have fixed the test cases for this scenario. For the race condition I wanted to have a quick syncup before writing the code, will probably like to do it in a separate post. |
@gauntface I talked to @philipwalton about the idea of handling the race condition in a separate PR. |
@@ -60,7 +80,7 @@ describe('request-queue tests', () => { | |||
|
|||
chai.assert(callbacks.requestWillEnqueue.calledOnce); | |||
chai.assert(callbacks.requestWillEnqueue.calledWith( | |||
sinon.match.has('request'))); | |||
sinon.match.has('request'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indentation of 4 spaces is intentional as it's a line continuation:
https://google.github.io/styleguide/jsguide.html#formatting-indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our lint doesn't catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the exception of a minor formatting issue and assuming the race condition will be handled in a separate PR.
@philipwalton @gauntface, fixed the whitespaces. Can we make some eslint rule to catch what @philipwalton mentioned? |
There's no built-in ESLint rule config that would catch this indentation issue. I have it on my list of rules to write, but it's very low on the list. |
R: @jeffposnick @philipwalton
Fixes #674
Fixes the repopulation of the list and also adds test coverage for it