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: adjust SecondParameter utility type #1125

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

victorboucher
Copy link
Contributor

Status

READY

Description

Fixes the poorly typed SecondParameter utility type.

@melloware
Copy link
Collaborator

Can you please open an Issue ticket explaining what is poorly typed and show us how this ticket makes it better?

@melloware
Copy link
Collaborator

@soartec-lab since this affects SWR can you review this change?

@melloware melloware added this to the 6.24.0 milestone Jan 13, 2024
@soartec-lab
Copy link
Member

@melloware

Of cource 👍 I'll review this, so could you assign me reviewer for this PR ?

Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

@victorboucher @melloware

This change will affect the case where global mutator is used, and I don't think it will affect the generated swr custom hook alone.
At the same time, we thought it would have the following advantages:

  • Can be expressed in a simpler and more understandable form
  • Being able to remove eslint disable

Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

(excuse me. I got a little confused and ended up editing the comment several times.)

I reviewed it. But i would like to confirm the scope of the effect a little more, so if possible, could you give me some time?

@melloware
Copy link
Collaborator

No problem just let me know what your final decision is.

@melloware melloware added the enhancement New feature or request label Jan 13, 2024
@soartec-lab
Copy link
Member

@melloware
Previously, never was returned if the second argument did not exist, but we investigated the impact of changing it to undefined.

In the first place, there will be no mismatch due to the case where the second argument does not exist, and even if it were to happen, there would be no problem because the type was specified as never | undefined in the function used. so, i agree with merging PR.

@melloware melloware merged commit 848bbe3 into orval-labs:master Jan 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants