Make datastore generics use lookup type #1
Make datastore generics use lookup type #1vidartf wants to merge 4 commits intosaulshanabrook:type-datastore-getfrom
Conversation
Only the type system uses this internally, for end users there should be no negative effect. A positive effect is better typing in change events.
3.4.5 seems to be minimal supported version with the code as is. It is possible that a lower version could be supported with some clever casting, but no tests done.
|
Note: I don't think that the "precompute" of the schema map as a generic arg is completely necessary (but it might be, not sure). I didn't try it out too much because I thought it was nicer to not have |
|
CI failure is because docs build are still using 3.x of phosphor and these changes require 3.4 at least. |
|
@vidartf could you give an example where this gives better type analysis than the original PR? |
| // for change events: | ||
| const data = change.change['test-schema-1']!['my-record']!; | ||
| expect(data.visible!.previous).to.equal(false); | ||
| expect(data.visible!.current).to.equal(true); |
There was a problem hiding this comment.
@saulshanabrook This bit! Without the typings for the change/patch structures I get errors like this:
Property 'visible' does not exist on type:
Change<
TestSchema | {
id: "test-schema-2";
fields: {
enabled: RegisterField<boolean>;
count: RegisterField<number>;
keywords: ListField<string>;
};
}
>In other words, the change.change['test-schema-1'] line isn't able to narrow it to Change<TestSchema>. Note that the test here was improved a bit to highlight this issue (the two schemas now have different types).
There was a problem hiding this comment.
Ah great! Thank you.
That makes sense, so we have to tie the ID on the change to the schema.
There was a problem hiding this comment.
Yeah, and the cleanest way I was able to find were with these guys: https://github.com/saulshanabrook/phosphor/pull/1/files#diff-e8ef547444f515352fab0946e93e81e5R648-R676
2 participants
Alternative to consider.