-
Notifications
You must be signed in to change notification settings - Fork 0
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(SyncExternalStore): Add StatefulSyncExternalStore to extend and provide structure #499
base: use-sync-external-store
Are you sure you want to change the base?
feat(SyncExternalStore): Add StatefulSyncExternalStore to extend and provide structure #499
Conversation
82c893a
to
0a7b2b5
Compare
export type StoreItem<TData> = { | ||
data: TData; | ||
error: null | string; | ||
state: typeof statefulStates; |
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.
This doesn't seem right... I don't think your typescript types are flowing through correctly. I need to pull this down...
}; | ||
}; | ||
|
||
export abstract class StatefulSyncExternalStore<TStore extends StoreState> extends SyncExternalStore<TStore> { |
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.
Can you create unit tests to help document usage? Also you know, code coverage?
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 pulled this down... in the original context-store there were three store types(copied from the readme):
- useContextStore - For non-indexable stores like objects and primatives or when you don't need to edit individual elements
- useIndexableContextStore - For indexable stores like maps or arrays with a single loading state at the root
- useIndexableStatefulContextStore - For indexable stores but when you want to maintain separate load states per item than the list of items
From the StoreState object you created, this looks to be specifically IndexableStateful
It seems it might make sense to have three classes then:
- StatefulSyncExternalStore
- IndexableStatefulSyncExternalStore
- IndexableStatefulItemSyncExternalStore
Or something...
type UpdateStoreParams<TStore extends StoreState, TParams = void, TResponse = void> = { | ||
key: keyof TStore; | ||
params?: TParams; | ||
service: (params: TParams) => Promise<TResponse>; |
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.
Nit, rename this to action
as it's not inherently tied to a service call. Also to make this more flexible consider a return type of (params: TParams) => Promise<TResponse> | TResponse;
so it can handle sync and async even though the sync scenario doesn't make a ton of sense
}; | ||
|
||
export abstract class StatefulSyncExternalStore<TStore extends StoreState> extends SyncExternalStore<TStore> { | ||
async updateStore<TParams, TResponse>(params: UpdateStoreParams<TStore, TParams, TResponse>) { |
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.
Okay playing with this the code I got to was something like this:
public getOne(id: string) {
return this.updateStore({
key: id,
service: (id: string) => Promise.resolve("hello" + id),
});
}
which is a bit weird that id
is reused soo many times. Also the only thing that changes here after initialization is id and params, service and updateSnapshot should be constants through the lifetime of the class.
I think there's some code reorganization needed to split these apart
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.
Maybe this is actually a good use of currying weirdly...
Edit: Nope, nevermind I think I'm wrong here..
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.
Here's the dumb non-currying version of some sample code which would be nice to be able to write (if we can get it be less lines of code anyway)
class SomeStore extends StatefulSyncExternalStore<Record<string, StoreItem<string>>> {
private getOneService = this.createUpdateStore({
service: (id: string) => Promise.resolve("hello" + id),
});
public getOne(id: string) {
return this.getOneService({
key: "id",
params: id,
});
}
}
function run() {
const d = new SomeStore({});
d.getOne("id-#1");
}
|
||
this.updateSnapshot(updateSnapshot.loading(key)); | ||
try { | ||
const data = await service(serviceParams as TParams); |
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.
This as TParams
screams either bard code implementation or bad typing... I think my suggestion to split up the function into a factory createUpdateStore
will help you fix this type
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 think the biggest changes required here is:
- Check out the difference between useContextStore, useIndexibleContextStore, and useStatefulIndexibleContextStore. You've implemented one of the three here.
UpdateStoreParams
can be split into two parts, the first is creation (service, updateSnapshot) and the second is action (key, params). If you split this, I think your types and readability get better.- Add a simple unit test so it's easy to see how you expect to use this in code
Once we iterate on that the last steps are:
- Add unit tests to get coverage as high as possible
- Add JSDoc if needed
- Add instructions in readme
…ExternalStore, and StatefulIndexableSyncExternalStore
0a7b2b5
to
f38c5f7
Compare
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
## [Unreleased] | |||
|
|||
- Added `ISyncExternalStore<T>` and `SyncExternalStore<T>` to make creating external stores for `React.useSyncExternalStore` easier (Requires React@18 or higher) | |||
- Add `StatefulSyncExternalStore` to provide structured extension of `SyncExternalStore`. |
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.
Leaving a note to update the CHANGELOG and README after we lock down the code
if (!Array.isArray(snapshot.data)) { | ||
throw new Error("IndexableSyncExternalStore: data is not an array"); | ||
} | ||
const index = snapshot.data.findIndex((data) => data.id === item.id); |
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.
Mmm not quite... you should snapshot.data should be an array (which is an object). You should be able to just do snapshot.data[data.id] = data
to set the data. This is a map lookup. Indexible means you are looking things based on a particular index-able value.
@@ -0,0 +1,49 @@ | |||
import { normalizeError } from "../shared/index.js"; |
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.
This one looks good to me... effectively do the same with the sync-external-store--indexable but move the state up a level and you're done... and also handle concurrent requests in your example code
@@ -0,0 +1,32 @@ | |||
import { normalizeError } from "../shared/index.js"; |
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.
This one looks good to me too, again concurrency is a good example to have somewhere in your test
No description provided.