Skip to content
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

fix: copy built-in awaited type #1242

Closed

Conversation

Newbie012
Copy link

@Newbie012 Newbie012 commented Jun 20, 2022

It fixes a bug when trying to use the atom in conjunction with generics. Given the following code:

const useX = <$StageName extends string>(params: {
    atom: WritableAtom<$StageName, SetStateAction<$StageName>, void>;
    stages: $StageName[];
  }) => {
    const [currentStage, setCurrentStage] = useAtom(params.atom);
    const stageNames = Object.keys(params.stages) as $StageName[];
    const currentStageNumber = stageNames.indexOf(currentStage) + 1;
}

I get the following error:

Argument of type 'Awaited<$StageName>' is not assignable to parameter of type '$StageName'.
  'Awaited<$StageName>' is assignable to the constraint of type '$StageName', but '$StageName' could be instantiated with a different subtype of constraint 'string'.
    Type 'unknown' is not assignable to type '$StageName'.
      '$StageName' could be instantiated with an arbitrary type which could be unrelated to 'unknown'.ts(2345)

Using the built-in Awaited type fixes this issue.

@vercel
Copy link

vercel bot commented Jun 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
jotai ✅ Ready (Inspect) Visit Preview Jun 20, 2022 at 2:56PM (UTC)

@codesandbox-ci
Copy link

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.

Latest deployment of this branch, based on commit a24684f:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
Next.js Configuration
Next.js with custom Babel config Configuration
React with custom Babel config Configuration

@dai-shi
Copy link
Member

dai-shi commented Jun 20, 2022

Thanks for reporting. I wasn't aware the issue with generics.
While your solution makes sense to fix the issue, it doesn't feel very ideal. We want to use the built-in one, right?
Would you be interested in contributing to https://github.com/sandersn/downlevel-dts ?

@Newbie012
Copy link
Author

Newbie012 commented Jun 21, 2022

Hey @dai-shi, sorry it took a long time to respond.

I agree. We should probably use the built-in type and let downlevel-dts do the extra work.
In fact, I tried to do it here but it turns out to be tricky since it's recursive, and I wasn't able to find a similar implementation.
For now, I've opened an issue sandersn/downlevel-dts#78 hoping I'll get a lead so I can merge it.
Although, I was wondering why did you redeclare the type in each module instead of exporting it once?

@dai-shi
Copy link
Member

dai-shi commented Jun 22, 2022

Although, I was wondering why did you redeclare the type in each module instead of exporting it once?

It's not only about this type, but I try not to export private types (public types are defined in ./src/index.ts), because we will change private types without notice, which breaks if people accidentally using private types (if it's exported).

@Newbie012
Copy link
Author

Interesting!
Anyway, not sure why I got so confused last night. I've opened a PR to downlevel-dts. Placing it here for a follow-up

sandersn/downlevel-dts#79

@Newbie012
Copy link
Author

@dai-shi I haven't got a response for the PR I made on downlevel-dts. In short, I've stumbled across this issue which I believe would be a problem merging this PR.

For now, I believe we have two choices - keep the current state and open an a new bug issue, or merge it until further development. Honestly, I'm not happy with either of these options 🙁.

@dai-shi
Copy link
Member

dai-shi commented Jun 24, 2022

keep the current state and open an a new bug issue

Let's do this. Closing this PR for now.

For your case, I think module augmentation would work.
Check this workaround: https://tsplay.dev/mLqX2W

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants