fix: Minor typing issues for web sandbox #3660
Conversation
| */ | ||
| export function isListenerObject(obj: TargetWithEvents = {}): boolean { | ||
| export function isListenerObject(obj: TargetWithEvents | any = {}): boolean { |
There was a problem hiding this comment.
Other options considered and open to
- Change the obj type to just any
function isListenerObject(obj: any = {}) - Add a generic type with a constraint of extending TargetWithEvents, but this requires the caller to cast as any
- Change the type checks to use "named"
obj["addEventListener"]lookup as (currently) TypeScript doesn't complain on this type of lookup - Cast each
objas any for usage(obj as any).addEventListener(just not pretty)
There was a problem hiding this comment.
I think I'd prefer
- Change the obj type to just any function isListenerObject(obj: any = {})
But it's not blocking for me. 🙂
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3660 +/- ##
==========================================
- Coverage 93.56% 93.55% -0.02%
==========================================
Files 275 275
Lines 8125 8126 +1
Branches 1691 1691
==========================================
Hits 7602 7602
- Misses 523 524 +1
|
| @@ -44,7 +44,7 @@ function getUrlNormalizingAnchor(): HTMLAnchorElement { | |||
| * @param key | |||
| */ | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| export function hasKey<O>(obj: O, key: keyof any): key is keyof O { | |||
| export function hasKey<O extends object>(obj: O, key: keyof any): key is keyof O { | |||
There was a problem hiding this comment.
Other options
- cast the usage only
return key in (obj as any); - remove the generic and just accept
anyforobj(not ideal)
| @@ -301,7 +301,7 @@ export class FetchInstrumentation extends InstrumentationBase< | |||
| ): Promise<Response> { | |||
| const self = this; | |||
| const url = web.parseUrl( | |||
| args[0] instanceof Request ? args[0].url : args[0] | |||
| args[0] instanceof Request ? args[0].url : String(args[0]) | |||
There was a problem hiding this comment.
For later version for TypeScript the args[0] is identified as a Request, string or URL and the URL can't be passed to the parseUrl so using the String() constructor to cause the URL variant to become a string and keep TypeScript happy.
| */ | ||
| export function isListenerObject(obj: TargetWithEvents = {}): boolean { | ||
| export function isListenerObject(obj: TargetWithEvents | any = {}): boolean { |
There was a problem hiding this comment.
I think I'd prefer
- Change the obj type to just any function isListenerObject(obj: any = {})
But it's not blocking for me. 🙂
MSNev
force-pushed
the
MSNev/SandboxFixes
branch
from
33b68e0
to
e55a52c
Compare
|
I mentioned during the SIG meeting that I'll merge it tomorrow, but given that the changes in this PR are minimal, I'll merge this now to unblock the sandbox. 🙂 |
The web sandbox is using a later version of TypeScript and these fixes are just typing updates to allow the sandbox to merge and compile.
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tested locally (on windows) and by duplicating the changed in a local sandbox branch