Repository navigation
Add URL to RequestInfo typedef - #1269
Conversation
|
|
|
Thanks for the PR! This section of the codebase is owned by Kagami Sascha Rosylight (@saschanaz) - if they write a comment saying "LGTM" then it will be merged. |
|
This sounds fair, but I wonder this could be breaking as some other libraries may be using |
|
Yeah, I trust past me to have done enough checking to make the call that we shouldn't do this last year, #1016 (comment) (likely because of ecosystem breakage) and I don't think anything's changed sinced then |
|
Please clarify: you're against allowing any random objects like |
|
Accepting URL for Web APIs is okay, what's not okay is to change the typedef and thus affect third-parties. I think changing the signatures of the APIs using |
| if (p.name.toLowerCase().includes("url") && p.type === "USVString") { | ||
| if ( | ||
| (p.name.toLowerCase().includes("url") && p.type === "USVString") || | ||
| p.type === "RequestInfo" |
There was a problem hiding this comment.
This is now a little bit long to be in a if clause, could you add a function for this and call it here?
There was a problem hiding this comment.
Sure.
Kagami Sascha Rosylight (saschanaz)
left a comment
Kagami Sascha Rosylight (saschanaz)
left a comment
There was a problem hiding this comment.
Looks good. Orta, wdyt?
(I'd prefer something like acceptsUrl btw)
| interface Cache { | ||
| add(request: RequestInfo): Promise<void>; | ||
| add(request: RequestInfo | URL): Promise<void>; | ||
| addAll(requests: RequestInfo[]): Promise<void>; |
There was a problem hiding this comment.
Not sure how to treat this one 🤔 but okay for now.
There was a problem hiding this comment.
I'm not sure how to properly implement this in generator either.
But in my opinion, even something like RequestInfo[] | URL[] would be an okay trade-off here: mixing those in userland code should be rarely needed.
Another option is to refactor this later by introducing some urlstring and RequestInfoOrUrl typedefs. Dunno about downsides.
These should be affected as well:
TypeScript-DOM-lib-generator/baselines/dom.iterable.generated.d.ts
Lines 25 to 27 in d060ef5
There was a problem hiding this comment.
Hmmmmmm. Let's merge this and tackle this in a separate patch.
|
lgtm |
|
Merging because Kagami Sascha Rosylight (@saschanaz) is a code-owner of all the changes - thanks! |
3 participants
Fixes: #1016
In most methods urlstring params are declared as
string | URL, which ensures perfect balance between safe code and implicit casts allowed forUSVString.This PR aligns
RequestInfowith them.In particular, it allows to pass
URLobjects infetch.