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

Improve type infererence for fromJS #1927

Merged
merged 2 commits into from Feb 3, 2023
Merged

Conversation

jdeniau
Copy link
Member

@jdeniau jdeniau commented Feb 3, 2023

Originally from KSXGitHub in #1617

With this pull request, fromJS(value) will no longer return any.

@jdeniau jdeniau merged commit 5e2379b into main Feb 3, 2023
@jdeniau jdeniau deleted the KSXGitHub-improve-from-js branch February 3, 2023 16:53
@jdeniau
Copy link
Member Author

jdeniau commented Feb 3, 2023

Thanks @KSXGitHub, I will release this next week probably.

@jdeniau
Copy link
Member Author

jdeniau commented Feb 6, 2023

This has been released in 4.2.4. Thanks !

@@ -5160,12 +5160,39 @@ declare namespace Immutable {
*/
function fromJS(
jsValue: unknown,
reviver?: (
reviver: (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No way this is correct - I never had problems skipping this argument in runtime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @ntucker . That's an overload for the definition right after. It does say :

  • if the reviver method is set, then the return type is Collection<unknown, unknown>
  • if the reviver is not set of undefined, then we can calculate the return type

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may very well be a bug with typescript, but I'm fairly certain you'd still be interested in supporting typescript versions less than 5.1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's continue in #1935 to avoid multiplying conversation in different threads

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.

None yet

3 participants