Data: Update types and fix type error #36190
Conversation
dmsnell
requested review from
gziolo,
jsnajdr,
sarayourfriend,
sirreal and
youknowriad
|
Size Change: 0 B Total Size: 1.09 MB ℹ️ View Unchanged
|
packages/data/src/types.ts
Outdated
| } | ||
|
|
||
| export interface WPDataReduxStoreConfig { | ||
| export interface StoreConfig< |
There was a problem hiding this comment.
I think this should have Redux in it to clarify that it's a special case for ReduxStores and not a generic interface.
There was a problem hiding this comment.
At several points I had that in there but I took it out each time because it seemed to make things more confusing than they need to be. I can add it back, ReduxStoreConfig, but I hope to have a better idea in the future to replace it. At one point I made ReduxStoreConfig extend StoreConfig but wasn't happy with that either.
What made me withdraw from Redux was that the way we use these configs is pretty much the same whether it's a Redux store or not and I was thinking that if they are the same then we could keep it as one concept and pass that around.
stevenlinx
added
[Type] Bug
|
|
||
| export interface StoreInstance< Config extends AnyConfig > { | ||
| getSelectors: () => SelectorsOf< Config >; | ||
| getActions: () => ActionCreatorsOf< Config >; |
There was a problem hiding this comment.
There's also the getResolveSelectors method that returns the promisifed selectors where one can await resolution. It's also part of the public stable API of StoreInstance.
All other fields are legacy.
There was a problem hiding this comment.
Thanks for clarifying. I wasn't sure if this belonged or if we wanted to expose it. All the calls I could find in practice only call this indirectly through resolveSelect() which seemed like it was maybe intentional that way.
Any thoughts on that?
There was a problem hiding this comment.
Also noted that we don't seem to expose this on generic stores or mention that it exists there. If we need something more specific for Redux stores maybe it'll be worth creating a ReduxStoreInstance
| export type AnyConfig = StoreConfig< any, any >; | ||
|
|
||
| export interface StoreInstance< Config extends AnyConfig > { | ||
| getSelectors: () => SelectorsOf< Config >; |
There was a problem hiding this comment.
I think using the SelectorsOf type here is incorrect. getSelectors returns bound selectors, without the state argument, while the Selectors in Config are unbound.
Similarly, getActions returns bound actions, and here it affects only the return value. For example, a bound action will never return a generator/iterator.
As long as the typedef is just Function this is all fine, but as soon as you try to specify the types more precisely there will be trouble.
Generally, for a particular store, getSelectors and getActions return generic interfaces that don't use any Redux or @wordpress/data types. For example:
interface InterfaceSelectors {
getSingleEnableItem( itemType: string, scope: string ): string | null;
isMultipleEnabledItemEnabled( itemType: string, scope: string, item: string ): boolean | undefined;
}
interface InterfaceActions {
setSingleEnableItem( itemType: string, scope: string ): void;
setMultipleEnableItem( itemType: string, scope: string, item: string, isEnable: boolean ): void;
}are both interfaces that any JS object could implement, they are just divided into "readers" and "writers" parts.
The StoreConfig types should be defined in terms of these public APIs, not the other way around. SelectorsOf<InterfaceSelectors, TState> should prepend a state argument to every method of InterfaceSelectors, and ActionCreatorsOf<InterfaceActions> should produce action creator types that correspond to the signatures of the bound action methods.
There was a problem hiding this comment.
thank you @jsnajdr - "getSelectors returns bound selectors, without the state argument, while the Selectors in Config are unbound" on this I agree but I don't want to address this yet. currently the config doesn't change the state of how these wind through the types because of what you identified - Function
in fact I'm trying to do this piecemeal to break the problem down and limit the size of any individual change.
the purpose therefore of SelectorsOf<Config> is to set us up so that later we can fill out that function without changing the config type. we can curry or bind the types of those selectors that are inferred through type mapping and SelectorsOf<Config> remains correct, but much more specific
SelectorsOf<InterfaceSelectors, TState> should prepend a state argument to every method of InterfaceSelectors, and ActionCreatorsOf should produce action creator types that correspond to the signatures of the bound action methods.
Exactly. I have working code for these, but in order to get there I've had to make more dramatic changes plus I've been dealing with issues where quirky things are happening and I think it's because TypeScript has hit its heuristic limit and given up.
The StoreConfig types should be defined in terms of these public APIs, not the other way around.
What I'm trying hard to do is define the store config implicitly and entirely through inference. So far it's working quite well and, for example, in order to fully-type a package like edit-post all we need to do is rename its files to .ts and add a tsconfig.json.
If you think that the SelectorsOf<Config> type is too misleading I could rewrite it to only keep the names
type SelectorsOf<Config extends AnyConfig> = Config extends StoreConfig<any, infer Selectors>
? { [ name in key Selectors ]: Function }
: never;that seems better while not changing things too much. if you like this I'll commit it.
There was a problem hiding this comment.
relaxed the constraints to the above in 0d8e9b4
packages/data/src/types.ts
Outdated
| } | ||
|
|
||
| export interface WPDataStore { | ||
| export interface StoreDefinition< Config extends AnyConfig > { |
There was a problem hiding this comment.
An alternative name that I think would make sense is StoreDescriptor. I like the analogy between POSIX file descriptors, where I do read( fd, buf, bufsize ), and reading from a store with registry.select( sd ). Both descriptors serve as opaque identifiers of the thing to read from.
There was a problem hiding this comment.
I'm fine with that. I chose "definition" to match the real usage I've seen in the codebase, but as is evident I'm also proposing renames to align those names better to the concepts and roles they fulfill.
I'll change it and if other people have different thoughts they can share them.
dmsnell
force-pushed
the
dmsnell/data-types-description
branch
from
b3cfbd5 to
0d7f29f
Compare
|
In 0d8e9b4 I've made some changes:
|
dmsnell
force-pushed
the
dmsnell/data-types-description
branch
from
0d8e9b4 to
c7ceda6
Compare
In this patch we're making a few updates to the primary types in the data module. The most important update is a fix. Previously the store config indicated that the actions and selectors should be an array of functions (or generators) but those should have been records or objects whose keys were the function names and whose values where the actual functions. These are equivalent to the type of the imported modules in practice. ```ts import * as selectors from './selectors'; type ConfigSelectors = typeof selectors; ``` The names for the core types have been altered to try and simplify their names and better reflect their usage in the data system.
dmsnell
force-pushed
the
dmsnell/data-types-description
branch
from
c7ceda6 to
9856149
Compare
|
Does anyone have any more thoughts on this? I would like to continue working on the types but don't want to get too far ahead of this change. |
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
I like this and believe it's good to go 👍
As a follow-up, sooner or later, we should refine the StoreInstance type. It should not be that the StoreInstance type is derived from the ReduxStoreConfig, but the other way around: StoreInstance is defined first, because it's the store's public API, and from that we derive what the ReduxStoreConfig needs to look like.
|
Somehow I missed your last comment @jsnajdr. Thanks for the review!
That makes sense. I've so-far played mostly with defining the selectors and actions separately, passing them in through the store configuration into the Redux store. |
Description
In this patch we're making a few updates to the primary types in the data module.
The most important update is a fix. Previously the store config indicated that
the actions and selectors should be an array of functions (or generators) but
those should have been records or objects whose keys were the function names
and whose values where the actual functions. These are equivalent to the type
of the imported modules in practice.
The names for the core types have been altered to try and simplify their names
and better reflect their usage in the data system.
How has this been tested?
There should be no built-code changes in this PR. Only the types have changed.
Types of changes
Updating the internal types in
data. This change is leading up to larger changesto add more useful typings to the package.
Checklist:
*.native.jsfiles for terms that need renaming or removal).