-
Notifications
You must be signed in to change notification settings - Fork 928
Fix packages/firestore/ usages of @ts-ignore. #1882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I left the equality_matcher.ts ones for the code we copy/pasted in, but fixed the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. One type suggestions for your new .d.ts
file.
@@ -244,8 +244,7 @@ export class WebChannelConnection implements Connection { | |||
// ReactNative and so we exclude it, which just means ReactNative may be | |||
// subject to the extra network roundtrip for CORS preflight. | |||
if (!isReactNative()) { | |||
// @ts-ignore | |||
request['httpHeadersOverwriteParam'] = '$httpHeaders'; | |||
(request as Indexable)['httpHeadersOverwriteParam'] = '$httpHeaders'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasted more time on this then I should have, but here you go:
interface WebChannelOptions {
messageHeaders?:StringMap;
initMessageHeaders?:StringMap;
messageContentType?:string;
messageUrlParam?:StringMap;
clientProtocolHeaderRequired?:boolean;
concurrentRequestLimit?:number;
supportsCrossDomainXhr?:boolean;
testUrl?:string;
sendRawJson?:boolean;
httpSessionIdParam?:string;
httpHeadersOverwriteParam?:string;
backgroundChannelTest?:boolean;
forceLongPolling?:boolean;
fastHandshake?:boolean;
disableRedac?:boolean;
clientProfile?:string;
internalChannelParams:{[key:string]:boolean|number};
xmlHttpFactory:unknown;
requestRefreshThresholds:{[key:string]:number};
}
const request: WebChannelOptions;
This is based on https://cs.corp.google.com/piper///depot/google3/javascript/closure/labs/net/webchannel.js?rcl=242544058&l=196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Added.
@hsubox76 If you're okay with this, I'll merge it into your PR. |
I left the equality_matcher.ts ones for the code we copy/pasted in, but fixed the rest.
The most controversial change is introducing
@Indexable
as a type alias for{[k: string]: unknown}
and casting to it in a few places. I could alternatively just cast directly to{[k: string]: unknown}
orany
(with ts-lint ignore comment) in those places...cc/ @hsubox76 FYI. I'll assign this to you once @schmidt-sebastian signs off.