Do not mix data and methods in sharedSelector #116
Conversation
| @@ -13,6 +13,15 @@ export interface EqualityCheck<T> { | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| export type Value = string | number | boolean | undefined | null | [] | Record<string, any> | |||
There was a problem hiding this comment.
Is this type to broad? The comment above says that functions is not supported, but Record<string, any> includes functions
I added type ValueWithoutFunction that does not accept functions
There was a problem hiding this comment.
If I understand correctly, consumers that are using the library will have to update their sharedFacets if they want to update to a version of the library that includes this fix, right? Or this is an optional change? I'm deducing this from the fact that selectors return OnlyDataOrOnlyMethods<V>.
If we go that far, I think we should probably do that in the sharedFacet function. Alternatively we could provide new sharedFacet and sharedSelector functions that could be used for people not having to immediately migrate upon adoption.
|
We talked a bit about this and figured out that it's probably best to create separate functions that implement the new types, to avoid breaking changes. We can mark the current shared facet functions as deprecated to remember to move away from them in the future. |
There was a problem hiding this comment.
Very nice, was really interesting to see how you solved it in typescript only! 👍
I do agree with @xaviervia 's comment though we should avoid breaking other people's code. However, I think we should locally run typechecking with the edited current sharedSelector to see where this ipotentially could cause the issues we had that gave rise to this task in the first place so we can nudge the teams responsible to migrate to the new 'sharedSelector' with typeguard.
Great work! 🙌
For now will click request changes, until we resolve the conversation with @xaviervia but I approve otherwise 👍
- Introduced safeSharedFacet, safeSharedSelector with safer type - Made generic type passed to sharedFacet read-only
|
A new facet API might be introduced soon which will render this PR obselete, consider taking up the effort again after such change is done if it is not already included in the scope of the API change. |
As discussed on FE tech collab 2023-02-28
This PR propose a new type that ensures that the return type of the selector does not mix methods and data
Discussion
Improvements