-
Notifications
You must be signed in to change notification settings - Fork 400
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(store): add tree shaking states into integration example #631
Conversation
What are the benefits of this approach, instead of previous While I am really keen of angular tree-shakeable services, it seems impossible to me to 'tree-shake' a state. Tree-shaking process isn't only about adding Beside that, it's true that this approach complies with "new angular way" for services - but why it uses keyword |
There was a typo in the You are totally right - you cannot tree shake any state, BUT only using native approach with We've recently published a package that allows you to get rid off actions (emitter), thus tree shaking works great with this approach. |
Awesome. Thank you for clarifying. |
@@ -31,6 +34,15 @@ export function State<T>(options: StoreOptions<T>) { | |||
meta.defaults = options.defaults; | |||
meta.name = options.name; | |||
|
|||
if (options.providedIn) { | |||
const provideIn: string | Type<unknown> = options.providedIn; |
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.
Change to providedIn
@splincode Doesn't the lazy example above result in a circular reference? |
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 feel like this feature is trying to solve something that is not a problem.
The providedIn
in angular is to provide tree shakeable services exposed from libraries so that when they are included in a client app by being injected into something they will be automatically registered with the root module.
Our state classes are a very different paradigm to services.
There is no clear benefit to NGXS from this feature.
I. Question
Answer: No. It works exactly the same as in Angular. Just injected into the module. Analogue: @NgModule({
imports: [NgxsModule.forFeature([ DetailState ])],
declarations: [DetailComponent]
})
export class DetailModule {} II. Question
I write huge applications consisting of a set of modules, sometimes components or modules are deleted and we forget to remove the state from the module. Together with @kyusupov33, we determined that the big problem is the implicit initialization of states. https://github.com/ngxs/schematics/blob/master/src/templates/starter-kit/store/store.config.ts It turns out that when we have a huge number of states, we are uncomfortable working with it. III. Question
Believe me, this feature is worthy to outshine even NGRX.
I am ready to write a huge number of tests for you to believe me. MotivationIf we usage Emitter plugin, we are improving for Tree Shaking, because since we use state class references @State({ })
export class AppState {
private static tagService;
@Emitter(AppStatusState.success)
public static success: Emittable<NormalizeTag>;
@Emitter(AppStatusState.failure)
public static failure: Emittable<string>;
constructor(tagService) {
AppState.tagService = tagService;
}
@Receiver()
public static request(ctx: StateContext<TagsStateModel>, { payload }: EmitterAction<NormalizeTag>) {
return this.tagService.addOne(payload.newTag).pipe(
tap(tag => this.success.emit({ tag: normalizeTag(tag), parentName: payload.parentName })),
catchError(error => this.failure.emit(error.message))
);
}
} |
@markwhitfeld Since I will not be able to get your approval, I will close this thread and later will conduct an experimental development. |
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?
#629
What is the new behavior?
Root provides
todos.state.ts
todo.state.ts
app.module.ts
app.component.ts
Lazy provides
detail.state.ts
detail.module.ts
detail.component.ts
Does this PR introduce a breaking change?
Other information
Update integration example with tree shaking states