-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
[RED-24] Feature/entity adapter draftable entity state #3669
[RED-24] Feature/entity adapter draftable entity state #3669
Conversation
improve typing to account for draftable entity state
…github.com/Olesj-Bilous/redux-toolkit.fork into feature/EntityAdapter-DraftableEntityState
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c8e899d:
|
Edit: For some reason
As for the API extractor, I couldn't find a build command in When I run it manually as
Also on query:
|
@Olesj-Bilous yeah, that docs build issue was due to another PR that got merged, which I've reverted. Also, the |
@Olesj-Bilous After some discussion, we'd like to put this into 2.0 instead of 1.9.x. That does mean this will need to be re-targeted at the |
@markerikson Alright I'll take care of that over the course of this week |
…EntityAdapter-DraftableEn...
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Okay, looks good! |
I got the go-ahead for this feature in this issue
Changes were restricted to types and typing in the
src/entities
directoryI introduced two new types:
Draftable Entity State
This type was used to update the methods of
EntityStateAdapter<T>
interface, e.g.addOne<S extends DraftableEntityState<T>>
(Perhaps we could lift S up to EntityStateAdapter, as
S extends DraftableEntityState<T> = EntityState<T>
? I left it unchanged for now)createStateOperator
was updated to reflect this change, which eliminated the need for a ts-ignoreDraftable Id Selector
The draftable id selector type seems necessary because
createUnsortedStateAdapter<T>(selectId: IdSelector<T>)
returns an adapter with methodupdateMany
updateMany
is generated withincreateUnsortedStateAdapter
by acreateStateOperator(updateManyMutably)
call wherebyupdateManyMutably
is set up to invoke a local functiontakeNewKey(keys, update, state): boolean
takeNewKey
gets the original entity fromstate
to see ifupdate
specifies a new id value:However, since I changed the type of state to extend DraftableEntityState, the second line needs to be
const updated: T | Draft<T>
and theselectId
parameter tocreateUnsortedStateAdapter
should beDraftableIdSelector<T>
selectId
parameter type to upstream functionscreateSortedStateAdapter
andcreateEntityAdapter
was updated accordinglyConclusion
While the
DraftableEntityState
addition seems harmless, theDraftableIdSelector
addition seems as though it may force clients to account for the possibility that themodel
parameter toselectId
isDraft<T>
when passingselectId
as a parameter tocreateEntityAdapter
However, as evidenced in the existing
tests/entity_state.test.ts
, in effect:The type-updated
createEntityAdapter
correctly recognizes selectId asDraftableIdSelector<BookModel>
Only if we try to call
createEntityAdapter
from a generic entitySliceEnhancer do we get an error'IdSelector<TModel>' is not assignable to type 'DraftableIdSelector<TModel>'
That is to say, only those who opt in to this newly available functionality - by implementing a generic entitySliceEnhancer - will have to take into account that
selectId
needs to beDraftableIdSelector<T>
If such an entitySliceEnhancer were to be implemented in accordance with the above, a concretely typed IdSelector will be valid TypeScript again
RED-24