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

When used with objects, the return type unifies all value types #16

Closed
novemberborn opened this issue Nov 12, 2020 · 6 comments
Closed

Comments

@novemberborn
Copy link
Contributor

const foo = await pProps({ bar: 'baz' as const, thud: 'qux' as const })

foo is:

type Foo = {
  bar: 'baz' | 'qux',
  thud: 'baz' | 'qux'
}

I would have expected it to be:

type Foo = {
  bar: 'baz',
  thud: 'qux`
}
@sindresorhus
Copy link
Owner

If anyone wants to work on this, see the initial attempt and feedback in #17.

@ChristianBoehlke
Copy link

We have a lot of type-casts in our codebase as well because of this issue and it looks like #17 would have worked well for us and the problem described here. Could you maybe explain a bit more what else you're looking for to accept a PR?

I understand that there is the advanced case with a mapper function that stayed unchanged in #17, but it did improve the "basic" use case considerably! I would also argue that when a mapper function is used, the types are probably okay like they are now because they would most likely be the same. On the other hand, if the object contains the promises directly the chances are pretty high (at least for us) that the types are different and it would be very beneficial if the return type would narrow them down correctly.

@karlhorky
Copy link

@ChristianBoehlke are you interested in doing a PR for this? Would be great to get this improvement!

@karlhorky
Copy link

karlhorky commented Aug 26, 2023

I was looking for alternatives, and found a few on npmtrends, but none that are recently released, popular and include TS types 🤔


I guess another alternative for fixing the types would be copying this TypeScript into your project, which removes the mapper and ability to use Maps and mimics the internals of p-props (also uses p-map):

import pMap from 'p-map';

export async function promiseProps<
  PromisesObject extends Record<string, Promise<unknown>>,
>(promisesObject: PromisesObject) {
  return Object.fromEntries(
    await pMap(Object.entries(promisesObject), async ([key, promise]) => {
      const result = await promise;
      return [key, result];
    }),
  ) as {
    [Key in keyof PromisesObject]: PromisesObject[Key] extends Promise<
      infer Result
    >
      ? Result
      : never;
  };
}

@Siilwyn
Copy link

Siilwyn commented Aug 28, 2023

Thanks for the ping @karlhorky, and even more for the types code! I've published https://www.npmjs.com/package/promise-all-props 3.0.0 with types.

@karlhorky
Copy link

@sindresorhus Thanks for 7eb631f 🙌

This was released as part of p-props@6.0.0. We just switched back to this version and can confirm that it's working 👍

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

Successfully merging a pull request may close this issue.

5 participants