feat(state): improve DX by logging undefined keys in selectSlice #852
Conversation
There was a problem hiding this comment.
Hi, here are my thoughts on this, I get the benefice of notifying the consumer from an undefined or partially undefined state instead of silently emitting nothing which can be hard to debug but, on the other side, lazy state initialization means undefined values and this is a behavior that happens often when using the state library, so this will warn a lot of correct usages. This is something that will really improve DX?
| tap((state) => { | ||
| if (isDevMode() && state === undefined) { | ||
| console.warn( | ||
| '@rx-angular/state#selectSlice WARNING: state is undefined' | ||
| ); | ||
| } | ||
| }), |
There was a problem hiding this comment.
Lazy state initialization is a key concept and warning for an undefined state is contradictory to that from my perspective.
There was a problem hiding this comment.
@edbzn true, lazy state is a key concept. the warning in devMode is still very helpful. especially when u are doing your first steps with rx-angular and cannot wrap your head around why selectSlice won't emit any values. The issue gets worse when your state interface has 100's of properties.
We have seen such cases, thats why the PR was opened :)
| console.warn( | ||
| `@rx-angular/state#selectSlice WARNING: undefined value(s) of selected key(s): "${undefinedKeys.join( | ||
| '", "' | ||
| )}" selectSlice operator will return undefined` |
There was a problem hiding this comment.
Technically, selectSlice will not return undefined because the next operator will filter undefined values, it will not emit any notification instead.
There was a problem hiding this comment.
agree, selectSlice won't emit any notification
|
@mrkpks thx for the PR! from my perspective it looks good besides the minor comment above. A further improvement would be a way to let it not run everytime. We should at first try it one of the demo applications and see if we get spammed too hard from it? |
Nx Cloud ReportCI ran the following commands for commit a587e51. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch
Sent with 💌 from NxCloud. |
|
Hi, first of all, thanks for your work. We discussed this and we think that it will spam too much the console as it is currently implemented. It's a very common scenario to have undefined keys with the lazy state initialization concept. We should find a way to avoid spamming when not desired. We came out with different alternatives:
*The generic operator should be used with the same array of keys and before the pipe(debug(['keyA', 'keyB']), selectSlice(['keyA, keyB']))This solution does not look very satisfying to me. **This is not a trivial problem, and I feel like there's no perfect solution to solve this. My position is to not augment the API surface and let the developer ensure keys are defined before selecting them. |
Fixes #639
Note: logs cases for
selectSliceoperator only. Behavior of this operator, when dealing with undefined slices, was the cause of confusion for developers most of the time