-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix(component-store): use asapScheduler to schedule lifecycle hooks check #3580
fix(component-store): use asapScheduler to schedule lifecycle hooks check #3580
Conversation
✅ Deploy Preview for ngrx-io canceled.Built without sensitive environment variables
|
@@ -322,7 +322,7 @@ export class ComponentStore<T extends object> implements OnDestroy { | |||
* but not used with provideComponentStore() | |||
*/ | |||
private checkProviderForHooks() { | |||
asyncScheduler.schedule(() => { | |||
asapScheduler.schedule(() => { | |||
if ( | |||
isDevMode() && |
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.
Additionally, I'm wondering whether the isDevMode()
check should be performed at the very beginning, guarding the whole asapScheduler.schedule
call, instead of just scheduled callback body
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 tried using isDevMode()
outside the callback body, and it doesn't work as intended.
I renamed the PR title to avoid potential confusion because we can use ComponentStore within the it('works before changes from this PR', fakeAsync(() => {
new ComponentStore();
tick(0);
}); // no error |
asyncScheduler.schedule(() => { | ||
asapScheduler.schedule(() => { |
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.
Thanks for the PR, @amakhrov! Can you add a unit test that verifies that there is no error?
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.
what if I just change tick(0)
to flushMicrotasks()
in the existing tests (there are 2 of them for the hooks feature).
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.
sounds good 👍
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 would still prefer a unit test though, the reproduction in the issue would be great.
This verifies that it solves that issue, and it also guard us against our future selves if we would change it back to another scheduler.
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.
@timdeschryver It's hard to test for that. I'd say the flushMicrotasks();
change in unit tests is the one that "reproduces" the issue.
fb028d6
to
b83f5cf
Compare
…heck This prevent "1 periodic timer(s) still in the queue" error by fakeAsync.
b83f5cf
to
46357a3
Compare
asyncScheduler.schedule(() => { | ||
asapScheduler.schedule(() => { |
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.
@timdeschryver It's hard to test for that. I'd say the flushMicrotasks();
change in unit tests is the one that "reproduces" the issue.
Switch from asyncScheduler to asapScheduler to prevent the "1 periodic timer(s) still in the queue" error.
PR Checklist
Please check if your PR fulfills the following requirements:
there are existing tests for this that still pass
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Closes #3573
What is the new behavior?
Does this PR introduce a breaking change?
Other information