refactor(router-state): change @Selector to createSelector
#2294
Conversation
|
View your CI Pipeline Execution ↗ for commit 9f62f20.
☁️ Nx Cloud last updated this comment at |
03977cb to
e63f170
Compare
@ngxs/devtools-plugin
@ngxs/hmr-plugin
@ngxs/form-plugin
@ngxs/storage-plugin
@ngxs/router-plugin
@ngxs/store
@ngxs/websocket-plugin
commit: |
|
Code Climate has analyzed commit e63f170 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 95.3% (0.0% change). View more on Code Climate. |
BundleMonUnchanged files (6)
No change in files bundle size Unchanged groups (1)
Final result: ✅ View report in BundleMon website ➡️ |
BundleMon (NGXS Plugins)Unchanged files (9)
Total files change -2B -0.01% Unchanged groups (1)
Final result: ✅ View report in BundleMon website ➡️ |
BundleMon (Integration Projects)Files added (1)
Files removed (3)
Total files change -139.36KB -67.77% Final result: ✅ View report in BundleMon website ➡️ |
In this commit, we switch from using the selector decorator to allow it to be dropped in production for apps that don't use it at all. Breaking change: To get the `RouterState.state`, it is now required to call the `state` function — `select(RouterState.state())`. Previously, it was possible to provide a generic type for the router state, e.g., `select(RouterState.state<CustomRouterState>)`, but this is not possible if `state` were a property.
e63f170 to
9f62f20
Compare
| static state<T = RouterStateSnapshot>() { | ||
| return createSelector([ROUTER_STATE_TOKEN], (state: RouterStateModel<T>) => { | ||
| // The `state` is optional if the selector is invoked before the router | ||
| // state is registered in NGXS. | ||
| return state?.state; | ||
| }); |
There was a problem hiding this comment.
@arturovt Isn't this a breaking change because it is updating the public API?
There was a problem hiding this comment.
Oh, I see that now that this is mentioned in the PR description. I think that there needs to be a note in the changelog about this breaking change.
Or do you think that there is an alternative like a separate getRouterState<T>() dynamic selector exposed by the plugin, and we leave the RouterState.state as a property.
OR is there the possibility of:
RouterState<RouterStateSnapshot>.state where the T on RouterState has a default so that RouterState.state still works.
There was a problem hiding this comment.
Hmmm.... I think I have figured out how to make this a non breaking change:
| static state<T = RouterStateSnapshot>() { | |
| return createSelector([ROUTER_STATE_TOKEN], (state: RouterStateModel<T>) => { | |
| // The `state` is optional if the selector is invoked before the router | |
| // state is registered in NGXS. | |
| return state?.state; | |
| }); | |
| static get state() { | |
| return createSelector([ROUTER_STATE_TOKEN], <T = RouterStateSnapshot>(state: RouterStateModel<T>) => { | |
| // The `state` is optional if the selector is invoked before the router | |
| // state is registered in NGXS. | |
| return state?.state; | |
| }); |
I made a StackBlitz example here:
https://stackblitz.com/edit/vitejs-vite-rrj1efj4?file=src%2Fcounter.ts
There was a problem hiding this comment.
Doesn't work, unfortunately:
In this commit, we switch from using the selector decorator to allow it to be dropped in production
for apps that don't use it at all.
Breaking change: To get the
RouterState.state, it is now required to call thestatefunction —
select(RouterState.state()). Previously, it was possible to provide a generic type forthe router state, e.g.,
select(RouterState.state<CustomRouterState>), but this is not possibleif
statewere a property.