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: mapping to preserve statically known types #17

Closed
wants to merge 6 commits into from

Conversation

buschtoens
Copy link

Fixes #16.

index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

It sounds like from #16 that the existing type should be changed instead of just adding an overload.

Base automatically changed from master to main January 23, 2021 18:07
@sindresorhus
Copy link
Owner

Friendly bump :)

@buschtoens
Copy link
Author

Holy fuck — how do you manage to keep track of stale PRs? 😲

I'll try to get to it later today. Thanks for the ping. 😊

@sindresorhus
Copy link
Owner

how do you manage to keep track of stale PRs?

This one had open in an old tab. But I check https://github.com/search?o=asc&q=is:pr+is:open+archived:false+user:@me&s=updated&type=Issues once in a while.

@buschtoens
Copy link
Author

buschtoens commented Mar 16, 2021

Looking at this again, I think the original types need to be kept in order to keep full support for mapper. My type overload only kicks in, if no mapper was passed, as I don't think that there is a way to allow the mapper to be taken into consideration, when the return type is constructed.

I may be wrong though.

But as it stands, this PR already significantly improves the usability of the types for the happy path (no mapper). So even if the mapper could be somehow taken into consideration, I think this PR would still be good to merge as-is.

What do you think?

/cc @novemberborn in case you are interested as well

index.test-d.ts Outdated Show resolved Hide resolved
Co-authored-by: Pedro Augusto de Paula Barbosa <papb1996@gmail.com>
index.test-d.ts Outdated Show resolved Hide resolved
index.test-d.ts Outdated Show resolved Hide resolved
@buschtoens
Copy link
Author

Tests are failing (yay!) and I will try to look at them locally some time today.

tbh this PR started as a mere drive-by quick fix via the GitHub web UI, but it seems I need to grant it some time in a real IDE. 😄

@sindresorhus
Copy link
Owner

Bump :)

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.

When used with objects, the return type unifies all value types
3 participants