-
-
Notifications
You must be signed in to change notification settings - Fork 697
fix: Wrong variance on the WritableAtom Result type parameter #3135
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
dai-shi
left a comment
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.
Thanks for digging into it. Yeah, SetSelf is tricky, and I'm not super happy with it.
I think using unknown is more appropriate. (If not, never? But if unknown works, it feels safer.)
|
@dai-shi Yeah, I flip flopped on this a couple times. My thinking was:
But I think either |
|
@atomanyih Sorry for the delay. On second thought, I think the original idea is to support this case. it('shoud type setSelf return value', () => {
function Component() {
const numberAtom = atom(0)
const writableAtom: WritableAtom<string, [], number> = atom(
(_get, { setSelf }) => {
const doubleNum = setSelf()
return String(doubleNum + 1)
},
(get, _set) => {
return get(numberAtom) * 2
},
)
expectType<WritableAtom<string, [], number>>(writableAtom)
}
expect(Component).toBeDefined()
})Can you reconsider the solution to support both? We may end up dropping either if we don't find any solutions. |
|
|
||
| it('WritableAtom Result type should be covariant', () => { | ||
| function Component() { | ||
| const writableAtom = atom(null, () => null) |
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 test doesn't represent your original reproduction. Can you do something similar to your repro in the PR description?
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.
Which part of the original reproduction do you mean? () => null or [someArg: number]?
The someArg was actually a mistake. That wouldn't be assignable anyway (the arguments should be contravariant). I fixed the example in my local reproduction and updated the PR description
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 mean in this case, we could do:
| const writableAtom = atom(null, () => null) | |
| const writableAtom = atom(null, () => null as null | number) |
I think we need the actual case in the test:
export const createWritableAtom = (): WritableAtom<
null,
[],
number | null
> =>
atom(
() => null,
(get, set, someArg: string) => {
return null;
},
);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.
☝️ Does it make sense?
Can you explain more about this use case? |
|
That is a very exceptional use case, we hack reading other atoms in our "read" function. It's probably used in some of third-party libraries. So, if we change the type from |
|
btw, I want to drop |
commit: |
|
| Playground | Link |
|---|---|
| React demo | https://livecodes.io?x=id/8296NM5XC |
See documentations for usage instructions.
|
/ecosystem-ci run |
Ecosystem CI Output |
dai-shi
left a comment
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'm considering to merge this for the next patch.
@atomanyih Please check comments.
|
|
||
| it('WritableAtom Result type should be covariant', () => { | ||
| function Component() { | ||
| const writableAtom = atom(null, () => null) |
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.
☝️ Does it make sense?
Reproduction:
Error:
This code should be valid: a function that returns
nullis assignable to a function that returnsnumber | null, and I would expect writable atom to work the same way.Explanation
From some digging, it seems like the issue is stemming from
Resultbeing propagated down to theSetSelfparameter ofReadinatom.tsI think what is happening is that, because
SetSelfis an argumentRead, typescript infers that it should be contravariant. That's why the assignability gets inverted from lines 3 to 4 of the error.Fix
I haven't fully examined the internal usage of
setSelf, so forgive me if this is totally off the mark. But it looks like the return value is not used in any of the call sites. So I think we could modify the typing ofWritableAtomto removeResultfrom read.This type change is saying, in effect, that we don't care about the return value of
SetAtomin theReadfunction.I've also added a type test to ensure the assignability of
WritableAtomremains correct.Let me know if I've gotten anything wrong!
Check List
pnpm run fixfor formatting and linting code and docs