Bug: `ExtractDispatchActions<unknown>` inferred from multiple models
hackape opened this issue · 12 comments
When pass in multiple models to useModel, if the "actions" field is missing in any model, the final merged actions is inferred as ExtractDispatchActions<unknown>. If supplied an empty object as "actions" value, the type is inferred correctly.
Reproduction:
const fooModel = model("foo").define({
state: {
foo: "foo",
},
// un-comment this line to obtain correct inferred type.
// actions: {}
})
const countModel = model("counter").define({
state: {
value: 1,
},
actions: {
add(state) {
return { ...state, value: state.value + 1 }
}
}
})
const [state, actions] = store.use(fooModel, countModel);
// ^ ts inferred type: const actions: ExtractDispatchActions<unknown>I'd like to suggest a quick patch here.
response._ = undefined as Omit<M, 'state'> & {
state: State extends void ? M['state'] : State;
+ actions: 'actions' extends keyof M ? M['actions'] : {};
}; A side question, since we're looking at reduck/packages/store/src/model/model.ts.
I'm kinda confused about the signature of this API model(name).define(modelDesc).
Why prefer model(name).define(modelDesc) over model({ name, ...modelDesc })? (possibly overloaded by model(name, modelDesc | modelInitializer))
At first glance I thought it's a builder pattern, but turns out it's not. By builder pattern I mean usage like:
const modelToBuild = model("name")
modelToBuild.define({ state });
modelToBuild.define({ actions });I don't feel this is the right decision, unless builder pattern is planned to be supported in the future. Is it planned?
Why prefer model(name).define(modelDesc) over model({ name, ...modelDesc })
At first I wanted to implement it in this way model({ name, ...modelDesc }), But the following TS type of problem was encountered
const todo = model<State>({
name: 'a',
state: { value: 1 }
actions: {
add(state) {
state.value++;
}
}
});
store.use(todo);type model<State> = (modelDesc: ModelDesc<State>) => Model<State>Here the Model cannot be derived from the real ModelDesc passed in by the user
type model<State> = (name: string) => { define: <M extends ModelDesc<State>>(modelDesc: M) => Model<M> }In this way, it is convenient to constrain the type of modelDesc by the generic type of the model function, and the final returned Model can contain the real type information of modelDesc, so that it is convenient to useModel to derive actions and state type.
To summarize, the use of model(name).define(modelDesc) is actually a trade-off to better support TS
+ actions: 'actions' extends keyof M ? M['actions'] : {};
I think it does fix what issue you report, cloud can raise a PR?, while I also found some other problem of type derive, I would fix it myself
+ actions: 'actions' extends keyof M ? M['actions'] : {};I think it does fix what issue you report, cloud can raise a PR?, while I also found some other problem of type derive, I would fix it myself
I have solved this problem in #31
"TS type problem was encountered"
I don't understand what exactly is the problem encountered? It's not obvious from your sample code.
I construct the following model function interface to showcase my preferred signature. It works in both cases, as seen below. You can either provide a State type argument to constrain ModelDesc, or you can omit it and fallback to infer the actual state type from ModelDesc.
And of course Model is derived just fine in both cases.
type Model<S> = {
(name: string): any;
_: Omit<ModelDesc<S>, 'name'>;
_name?: string;
};
interface ModelDesc<State = any> {
name: string;
state: State;
actions?: Actions<State>;
}
type Actions<S> = {
[actionType: string]: (state: S, ...args: any) => any
};
function model<S>(modelDesc: ModelDesc<S> | ((ctx: any, hooks: any) => ModelDesc<S>)): Model<S> {
return undefined as any
}
// usage
type State = { value: number };
const todo1 = model<State>({
name: 'a',
state: { value: 1 },
actions: {
add(state) {
state.value++;
}
}
});
const todo2 = model({
name: 'a',
state: { value: 1 },
actions: {
add(state) {
state.value++;
}
}
});Playground
@hackape You can try to implement a simple type of useModel based on the above code
OK, here's my experiment.
I copy-paste everything from /packages/store/src/types/model.ts, and apply only one modification to type Model to add a generic type param <S> for State. Everything else stays the same.
type Model<S=any> = {
(name: string): any;
_: Omit<ModelDesc<S>, 'name'>;
// model name
_name?: string;
};Then I use function model<S>(modelDesc: ModelDesc<S> | ((ctx: any, hooks: any) => ModelDesc<S>)): Model<S> to create 2 models, just like in earlier code snippet. Turns out they work just fine with current useModel function interface.
I don't encounter any problem. 😕 So...I still don't get what exact is the TS problem that you mentioned. Did I miss anything?
To be crystal clear, I'm not hard-selling anything here. I understand that even if I had a good point, the API is likely to stay since you've already shipped v1 and breaking change is unlikely to happen.
I had this confusion reading the source code, and here's me sharing my thoughts and that's all. Like I said it's a side question /discussion, not a problem that needs fixing.
I've seen the implementation here, it seems not working well on deriving the type of actions. My previous implementation was similar to yours, but the automatic derivation of the actions(returned in useModel) type when passing the generic type State bothered me for a long time, so I settled for model<State>(name).define({}) instead.
So...I still don't get what exact is the TS problem that you mentioned. Did I miss anything?
Maybe the language is difficult to describe, I'll put together a TS demo later
Sure, when you got the time. And if you don’t then just leave it, it’s not that important anyway. I kinda feel sorry for holding yon up for too long just to satisfy my curiosity 😅