[feat]: parseAs options reports correct data type

[HugeLetters]

Changes

Closes #1370

How to Review

Improves typing of fetch function by introducing a fetchOptions generic which can then be used to report correct return type.

Checklist

• Unit tests updated
• docs/ updated (if necessary)
• pnpm run update:examples run (only applicable for openapi-typescript)

[changeset-bot]

⚠️ No Changeset found

Latest commit: c2b2da7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[HugeLetters]

@drwpow I've made a draft PR which only changes GET requests - seem to work fine. Could you check if it looks fine to you? If yes, I'll update to include over http verbs too.

In general I think this change makes API more flexible in the future since any option property can be used now to modify return type.

[drwpow]

Could you check if it looks fine to you? If yes, I'll update to include over http verbs too.

ParseAsResponse looks great to me—those look like the correct types. This looks like the right direction—now it’s just plugging everything up and making sure there aren’t any regressions in the test suite where parseAs is still the default (JSON)

[HugeLetters]

@drwpow ok, I'm a bit stumped at the moment - this pattern doesn't seem to work for some requests which require body/params. I've been able to pinpoint the root of the issue somewhat but have no idea what's causing it - seems like literal types cause issues (like string or number are ok, but 1 or "1" are not). Here's a video with an example - I can also link my schema file here, I'm not using the one from the examples but the one from my project since it has routes for other verbs.
Screencast from 06-11-23 11:44:32.webm

And because literal types cause these issues function call to POST/DELETE etc falls back to use default generic parameter value(which is the value from extends clause) so you get no return type inference.

The thing is I don't see anywhere in the code anything which checks for values to be strings or numbers so I don't really have an idea on where to look. I don't even see anything which checks for value beyond 'content' prop so why would it matter which values does it have inside of it.

[drwpow]

Yeah this is a tricky problem for sure. Will dig in later this week; this will take some investigation

[HugeLetters]

@drwpow in case that would help - when I made a wrapper for our project for making api calls I also had an issue with POST & PATCH verbs where it would say that my provided fetchOptions doesn't extend type FetchOptions<any> - so after lotta trial and error I've made this type to represent the default basic defaultOptions. Perhaps this might help a bit. I believe the issue was caused because post and patch requests had routes which required params and body in them but that's a bit of a guess.
I've tried to use that type here as well but didn't help.

type Options =
	// Post requests
	| FetchOptions<{ requestBody?: { content?: { [x: `${string}/${string}`]: unknown } } }>
	// Patch requests
	| FetchOptions<{
			requestBody?: { content?: { [x: `${string}/${string}`]: unknown } };
			parameters: unknown;
	  }>
	// Get requests & the rest
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	| FetchOptions<any>;

[drwpow]

This is mostly unrelated, but problems like this are why in openapi-typescript@7 there are a lot more requestBody?: never shapes (and the like) because it reduces the inference from hundreds of possible permutations of requests to a much smaller number. My fear was for more complicated things like this, TypeScript would just fail when its inference was trying to create unions internally that were too complex for it.

Not saying that 7.x will solve this problem for us or anything—it’s the same problem here either way—but just saying I’ll probably first try and solve this in the 7.x schema types since it theoretically should be simpler, then backport it to 6.x types.

[HugeLetters]

@drwpow sure, if you think it will be much easier to work with new types I can wait till these changes are made and make my PR around them.
I don't think this particular parseAs is urgent for anyone anyways - the more important change of this PR probably is transition to options generic which would allow to add new options in the future which would also allow to modify return type in some new ways.

[drwpow]

OK finally got some time to take a crack at it in #1442. In general it’s hard to explain what I did, and there’s also some arbitrary cleanup I did that just made it easier for me to debug, but didn’t actually contribute to the solution.

But the short version is your PR laid the correct groundwork but the inference was getting lost from the GET() function to the return type. If ...init has type O, and we try and pass that as a generic argument all the way down (FetchResponse<T, O>), it’s really easy for TypeScript (or us) to get confused at the difference between init’s generic typing versus its runtime value. Often TS will try and reuse generic typing because it’s cheaper. So at the end, when it tried to look up parseAs, it essentially “forgot” what the runtime value was, and instead, we ended up with a union of all the possible response types which was an unresolvable union.

Instead, we want FetchResponse<T, (typeof init)['parseAs']> because this essentially has to infer the actual value of init.parseAs, and it will pass that down through all the helpers correctly.

I’m not saying that there’s not a way to make O work here (the entire FetchOptions shape), but the way to do it involves stacking complex inferences in a way that’s tough to write and tough to maintain. But saving a primitive value is much easier to deal with. And that’s all my PR did—just flattened parseAs into its runtime value, and passed that along. The rest of the changes were, like I said, just cleanup that made the problem easier to see, and hopefully cleaned up some duplication.

I think we’ll have to test it a little more, but it’s close!

[drwpow]

Anyway, feel free to cherrypick #1442 and/or pull that into your PR, and test it some more on your end.

[HugeLetters]

Ok, so the current iteration passes my mini test-suite I've set up with a more complicated openapi schema - I'll give it a day or two to try to make it work with an Options object as I was trying before.
If not - I think it's okay as is.

[HugeLetters]

This is pure insanity but I think I got it solved at least, haha

leproblem.mp4

[drwpow]

This is pure insanity but I think I got it solved at least, haha
leproblem.mp4

wait—why is const necessary here? are those 2 fields an enum (edit: in the OpenAPI schema)?

[HugeLetters]

This is pure insanity but I think I got it solved at least, haha
leproblem.mp4

wait—why is const necessary here? are those 2 fields an enum (edit: in the OpenAPI schema)?

Yup, those two fields are enums - seems like TS inference isn't working properly here and it thinks grade and title are just string in provided object unless I put as const on them. At least now we know it's not the issue on our end.
Extracting the value to a variable and annotating its type or just asserting the full type on an inline object works too .

A friend of mine showed me this and I think it's a related issue.

Screen.Recording.2023-11-28.at.16.58.12.mov

[drwpow]

Sorry—was there anything else you wanted to get in before merging? I think this looks great as-is, and happy to merge and kick the tires some more. But wanted to double-check just in case.

[HugeLetters]

@drwpow Hi, yup, I'm done