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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect type inference in v0.21.0 #266

Closed
Sec-ant opened this issue Nov 23, 2023 · 8 comments
Closed

Incorrect type inference in v0.21.0 #266

Sec-ant opened this issue Nov 23, 2023 · 8 comments
Assignees
Labels
bug Something isn't working priority This has priority workaround Workaround fixes problem

Comments

@Sec-ant
Copy link

Sec-ant commented Nov 23, 2023

Hello,

I was able to use this 馃憞 code to parse a string into a number in v0.20.1 and get the correct type for n (number):

const n = parse(
  transform(string(), (input) => parseInt(input, 10), [minValue(0)]),
  "1",
);

But when I updated to v0.21.0, the inferred type of n of the same code became string | number | bigint | boolean | Date.

I have to change it to this 馃憞 to fix the type.

const n = parse(
  transform(string(), (input) => parseInt(input, 10), [minValue<number, number>(0)]),
  "1",
);

Did I miss something?

Thanks

@fabian-hiller
Copy link
Owner

We did not change anything in transform. However, we have made changes to the validation functions such as minValue. I will check if this causes the problem and if there is a solution. What you can do alternatively until this is fixed is to pass a schema with a pipe instead of just a pipe as the third argument.

const n = parse(
  transform(string(), (input) => parseInt(input, 10), number([minValue(0)])),
  '1'
);

@fabian-hiller fabian-hiller self-assigned this Nov 23, 2023
@fabian-hiller fabian-hiller added bug Something isn't working priority This has priority labels Nov 23, 2023
@fabian-hiller
Copy link
Owner

If you stick with the generics as a workaround, I recommend changing the second generic to the first argument of minValue:

const n = parse(
  transform(string(), (input) => parseInt(input, 10), [minValue<number, 0>(0)]),
  "1",
);

@fabian-hiller fabian-hiller added the workaround Workaround fixes problem label Nov 23, 2023
@Sec-ant
Copy link
Author

Sec-ant commented Nov 23, 2023

I can use the schema workaround for now, it helps, thanks!

@fabian-hiller
Copy link
Owner

Unfortunately, I haven't found a solution other than the workaround, and I'm not sure there is one. The problem is that TypeScript no longer resolves the types correctly since the internal change from a function to an object. I'll take another look at the problem in the next few weeks.

@Sec-ant
Copy link
Author

Sec-ant commented Dec 11, 2023

Thanks for the effort. I'm totally fine with the workaround. But IMO if using pipes as the third argument of transform is an allowed usage maybe this should be addressed somewhere if not here? Is this an issue that should be raised and fixed in TypeScript? Or is there already one where I can track?

@fabian-hiller
Copy link
Owner

Yes, that is why I did not close this issue. I think it is not a TypeScript bug, just a different behavior than expected on our end. I will investigate this again in the coming weeks. If there is no solution, we could simply not allow a pipe as a third argument.

@Sec-ant
Copy link
Author

Sec-ant commented Dec 11, 2023

Again, thanks for your effort and appreciate your time! @fabian-hiller

@fabian-hiller
Copy link
Owner

This should no longer be a problem with the latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority This has priority workaround Workaround fixes problem
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants