undici icon indicating copy to clipboard operation
undici copied to clipboard

fix(fetch): improve Headers and Request type-compatibility

Open kettanaito opened this issue 2 years ago • 3 comments

  • Reopened #1947 because GitHub UI is fantastic.

kettanaito avatar Feb 22 '23 11:02 kettanaito

As mentioned in the previous review, a type test has to be added here. I will add it this/next week due to the work load.

kettanaito avatar Feb 22 '23 11:02 kettanaito

type tests are failing (npm run test:typescript)

   test/types/fetch.test-d.ts:141:0
  ✖  141:0  Parameter type string is declared too wide for argument type ReferrerPolicy.  

KhafraDev avatar Mar 10 '23 15:03 KhafraDev

Suddenly after an update in my package i got the problem with the referrer property missing and i just found this library and this PR is related.

I manually edited fetch.d.ts to test this PR. I can get rid of the test error you mentioned if i revert

readonly referrerPolicy: ReferrerPolicy

to

readonly referrerPolicy: string

  • Would that be acceptable ? I fear this is not strict enough or just a bad solution
  • Should i make another PR because the person who submitted it seems to be busy?

MikeZeDev avatar Oct 12 '23 14:10 MikeZeDev

@MikeZeDev, not sure if string isn't too broad for the referrer policy. It is annotated as RefererPolicy type in lib.dom, I think it'd be best if Undici introduced compatibility with that as well. Value-wise, you cannot set unknown values anyway. Can you tell me more about the use case that resulted in the error for you?

kettanaito avatar May 01 '24 10:05 kettanaito

@KhafraDev, I've fixed the test by adjusting the expected type. It's no longer a wider string but a narrower ReferrerPolicy, which is compatible with how lib.dom annotates the referrerPolicy request property.

kettanaito avatar May 01 '24 10:05 kettanaito

Type tests are passing now.

> [email protected] test:typescript
> tsd && tsc --skipLibCheck test/imports/undici-import.ts

@KhafraDev, may I please get the workflow approved on this pull request to see if there are any other pending things to resolve? Thanks.

kettanaito avatar May 01 '24 10:05 kettanaito

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.17%. Comparing base (e009b0c) to head (af9433a). Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1964   +/-   ##
=======================================
  Coverage   94.17%   94.17%           
=======================================
  Files          90       90           
  Lines       24434    24434           
=======================================
  Hits        23011    23011           
  Misses       1423     1423           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 01 '24 13:05 codecov-commenter

@mcollina, note that this change can be technically breaking if consumers relied on referrerPolicy being a plain string. A narrower type shipped by this change will error on the type level in some cases.

My personal take on situations like this is if you rely on a bug, you should expect breaking changes. I'd release this as a patch fix since the type annotations were previously incorrect.

kettanaito avatar May 01 '24 14:05 kettanaito