-
-
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
feat(component-store): add OnStoreInit and OnStateInit lifecycle hooks #3368
Conversation
✅ Deploy Preview for ngrx-io ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
// State can be initialized either through constructor or setState. | ||
if (defaultState) { | ||
this.initState(defaultState); | ||
} | ||
} | ||
|
||
private checkInitStoreHook() { |
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.
suggestion: consider a different name:
triggerInitStoreEvent
callInitStoreHook
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.
Good suggestions
@@ -18,6 +19,7 @@ describe('Login Page', () => { | |||
imports: [NoopAnimationsModule, MaterialModule, ReactiveFormsModule], | |||
declarations: [LoginPageComponent, LoginFormComponent], | |||
providers: [ | |||
LoginPageStore, |
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.
issue: cover OnStateInit
and OnStoreInit
hooks with tests
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.
Agreed. This was for testing example code
45c853f
to
47ea64f
Compare
7070b02
to
e7ac13e
Compare
Code is refactored, tests are updated and green. I do have some concerns about the feature in general though. Because we are chaining off the constructor of the ComponentStore, it forces us or the developer to consider workarounds to use it effectively. Consider the example below: import { Injectable } from '@angular/core';
import {
ComponentStore,
OnStateInit,
OnStoreInit,
} from '@ngrx/component-store';
import { delay, of, tap } from 'rxjs';
@Injectable()
export class LoginPageStore
extends ComponentStore<{ test: boolean }>
implements OnStoreInit, OnStateInit
{
props = [];
constructor() {
super();
}
logEffect = this.effect(
tap<void>(() => {
console.log('effect');
})
);
ngrxOnStoreInit() {
this.logEffect(); // doesn't work because constructor isn't done
console.log(this.props); // undefined
}
ngrxOnStateInit() {
console.log('state init', this.get()); // works
this.logEffect(); // blows up
}
} The examples below work import { Injectable } from '@angular/core';
import {
ComponentStore,
OnStateInit,
OnStoreInit,
} from '@ngrx/component-store';
import { delay, of, tap } from 'rxjs';
@Injectable()
export class LoginPageStore
extends ComponentStore<{ test: boolean }>
implements OnStoreInit, OnStateInit
{
props = []
constructor() {
super();
}
logEffect = this.effect(
tap<void>(() => {
console.log('effect');
})
);
ngrxOnStoreInit() {
of(null)
.pipe(delay(0))
.subscribe(() => {
console.log('store init');
console.log(this.props); // empty array
this.logEffect(); // works
});
}
ngrxOnStateInit() {
setTimeout(() => {
console.log('state init', this.get()); // works
this.logEffect();
});
}
} From my understanding, setTimeout is a macrotask that gets pushed into the queue, running on the next "tick" which allows it to run successfully. It would put the responsibility on the developer to run the code inside the hooks in a way that doesn't break. I'm open to other suggestions also internally we could make to make it more predictable without introducing race conditions. If Angular had some sort of |
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.
Hoisting 😳 The behavior could be very confusing for users.
This will work for component stores that are explicitly provided in components, modules, or route config:
const WITH_HOOKS = new InjectionToken<ComponentStore<any>[]>(
'@ngrx/component-store: ComponentStores with Hooks'
);
export function provideWithHooks(
componentStoreClass: Type<ComponentStore<any>>
) {
return [
{ provide: WITH_HOOKS, multi: true, useClass: componentStoreClass },
{
provide: componentStoreClass,
useFactory: () => {
const componentStore = inject(WITH_HOOKS).pop();
if (isOnStoreInitDefined(componentStore)) {
componentStore.ngrxOnStoreInit();
}
if (isOnStateInitDefined(componentStore)) {
componentStore.state$
.pipe(take(1), takeUntil(componentStore.destroy$))
.subscribe(() => componentStore.ngrxOnStateInit());
}
return componentStore;
},
},
];
}
Example:
@Injectable()
class LifecycleStore
extends ComponentStore<LifeCycle>
implements OnStoreInit, OnStateInit
{
constructor() {
super({ init: true });
console.log('constructor');
this.logEffect('constructor effect');
}
logEffect = this.effect(
tap<string>((message) => {
console.log(message);
})
);
ngrxOnStoreInit(): void {
console.log('onStoreInit');
this.logEffect('store init effect');
};
ngrxOnStateInit(): void {
console.log('onStateInit');
this.logEffect('state init effect');
}
}
@Component({
providers: [provideWithHooks(LifecycleStore)]
})
export class AppComponent {
constructor(private readonly lifecycleStore: LifecycleStore) {}
}
// console:
// constructor
// constructor effect
// onStoreInit
// store init effect
// onStateInit
// state init effect
Nice, I thought of adding a type of |
e7ac13e
to
c95d9d4
Compare
I refactored the logic for the init checks to run after the constructor is complete, so the additional token isn't required and it runs consistently whether it's in |
3f36f5d
to
96a7669
Compare
603b82f
to
9f1a690
Compare
a74cbbc
to
3c81a95
Compare
3c81a95
to
39f3e52
Compare
39f3e52
to
86d19dd
Compare
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 🎉 The only thing I would change is not to expose isStoreInitDefined
and isStateInitDefined
guards to the public API.
export function provideComponentStore( | ||
componentStoreClass: Type<ComponentStore<any>> | ||
): Provider[] { | ||
const CS_WITH_HOOKS = new InjectionToken<ComponentStore<any>>( |
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.
const CS_WITH_HOOKS = new InjectionToken<ComponentStore<any>>( | |
const CS_WITH_HOOKS = new InjectionToken<ComponentStore<unknown>>( |
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 unknown
will fail extends object
constraint on ComponentStore
generics. If we want to absolutely avoid any
, I think {}
would do it?
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.
yeah, {}
or <object>
. Just not any
:)
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.
👍
d68f4b7
to
247f6e0
Compare
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 (again 😅)
Should we document somewhere why it's done like this instead of "just" adding the hooks to the Store class? (or will this be a part of the docs?).
@timdeschryver yes, I'm going to add a docs page in a follow-up PR. We're also looking at adding a warning if the lifecycle hooks are defined but it wasn't provided through the function. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Closes #3335
What is the new behavior?
Adds two new lifecycle hooks to ComponentStore along with their respective interfaces
.get()
method because the state hasn't been set, even if done eagerly.Adds a new exported function
provideComponentStore(ComponentStore)
that is used to provide the ComponentStore, instantiate it, and run the lifecycle hooks if definedDoes this PR introduce a breaking change?
Other information