use substring for content type parsing - #1372
Conversation
|
|
I tried to create a test for that issue in |
There was a problem hiding this comment.
Pull Request Overview
Updates TypeScript utility types to use substring matching instead of exact key matching for content type parsing, addressing issue #1370 where content type parsing failed for media types with additional parameters.
- Modified
OmitAlltype to use substring pattern matching instead of exact key comparison - Updated
IfHasKeytype to check for substring matches in object keys rather than exact key presence
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| ? Extract<keyof SomeObject, `${string}${FirstKey}${string}`> extends never | ||
| ? IfHasKey<SomeObject, RestKeys, Yes, No> | ||
| : Yes | ||
| : No; |
There was a problem hiding this comment.
The logic appears inverted. When a key is found (Extract returns non-never), the condition should return Yes, but currently it returns No via the IfHasKey recursive call. Consider changing to: Extract<keyof SomeObject, \${string}${FirstKey}${string}`> extends never ? IfHasKey<SomeObject, RestKeys, Yes, No> : Yes`
| : No; | |
| : No | |
| : Yes; |
There was a problem hiding this comment.
@ihor-rud I requested the Copilot review, but I'm uncertain about its suggestion regarding the inverted logic. Do you have any comment about this? I guess this is a great reason to have a test. If you have access to CoPilot I'd go ahead and ask it to write a test around this logic change as a first start on tests. If I'm able to free up time later this week I'll take a shot at writing a test if you (or Copilot) haven't gotten there first.
There was a problem hiding this comment.
I requested the Copilot review, but I'm uncertain about its suggestion regarding the inverted logic. Do you have any comment about this?
Copilot is wrong here. Extract<keyof SomeObject, ${string}${FirstKey}${string}> returns never when there is no overlap between 1 and 2 arguments, so we check rest of keys.
I guess this is a great reason to have a test
I can try to write a type level test using tsd. I have no idea how to test it with jest in runtime because types will be gone at this point.
There was a problem hiding this comment.
I made code more straightforward in last commit. Tests turnout to be more complex then I expected. The biggest issue is where to put them. OmitAll and IfHasKey are not part of public api. anyway here is how tsd tests could look like
https://gist.github.com/ihor-rud/5baaf3b4f3bac4166753c04111059ef1
There was a problem hiding this comment.
We don't have tests for types yet. I would really like to get that going with something like tsd.
Once we get it going I believe an LLM could write most of the tests for us. We wouldn't need to check the LLM's work that closely, because if the tests pass, they're by definition correct.
With test coverage in place we will have the opportunity to refactor the types and make them easier to understand.
There was a problem hiding this comment.
@pmcelhaney I just added tsd tests. you can run them using this command
yarn build && yarn test:tsd
|
Great contribution, thank you very much! Merging now and then I'll get a release out. |
changeset for revert change in #1372 which broke type shortcuts like .json(), .html(), etc.
|
Hey @ihor-rud, it looks like this change actually broke the shortcuts. Instead of detecting |
fix for #1370