Skip to content

Add typings for WebChannel #2385

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 3, 2019
Merged

Add typings for WebChannel #2385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a864185
Add Types for Webchannel
schmidt-sebastian Nov 28, 2019
e9c7d01
Review comments
schmidt-sebastian Dec 2, 2019
82f6645
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Dec 2, 2019
dc0b8a5
Update index.d.ts
schmidt-sebastian Dec 2, 2019
3f76270
More review feedback
schmidt-sebastian Dec 3, 2019
2936fec
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Dec 3, 2019
1f4767b
Merge branch 'mrschmidt/webchannel' of github.com:firebase/firebase-j…
schmidt-sebastian Dec 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions packages/firestore/src/platform_browser/webchannel_connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
ErrorCode,
EventType,
WebChannel,
WebChannelError,
WebChannelOptions,
XhrIo
} from '@firebase/webchannel-wrapper';
Expand Down Expand Up @@ -98,9 +99,7 @@ export class WebChannelConnection implements Connection {
const url = this.makeUrl(rpcName);

return new Promise((resolve: Resolver<Resp>, reject: Rejecter) => {
// XhrIo doesn't have TS typings.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const xhr: any = new XhrIo();
const xhr = new XhrIo();
xhr.listenOnce(EventType.COMPLETE, () => {
try {
switch (xhr.getLastErrorCode()) {
Expand All @@ -125,7 +124,8 @@ export class WebChannelConnection implements Connection {
xhr.getResponseText()
);
if (status > 0) {
const responseError = xhr.getResponseJson().error;
const responseError = (xhr.getResponseJson() as WebChannelError)
.error;
if (
!!responseError &&
!!responseError.status &&
Expand Down Expand Up @@ -272,9 +272,7 @@ export class WebChannelConnection implements Connection {

const url = urlParts.join('');
log.debug(LOG_TAG, 'Creating WebChannel: ' + url + ' ' + request);
// Use any because listen isn't defined on it.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const channel = webchannelTransport.createWebChannel(url, request) as any;
const channel = webchannelTransport.createWebChannel(url, request);

// WebChannel supports sending the first message with the handshake - saving
// a network round trip. However, it will have to call send in the same
Expand Down Expand Up @@ -310,14 +308,14 @@ export class WebChannelConnection implements Connection {
// Note that eventually this function could go away if we are confident
// enough the code is exception free.
const unguardedEventListen = <T>(
type: WebChannel.EventType,
type: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it not possible to make WebChannel.EventType work? It seems like we're losing a bit of help from the type system here by switching to just string. Not the end of the world though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebChannel.EventType is now a variable declaration and no longer a string. I had tried both WebChannel.EventType and typepof WebChannel.EventType.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bleh... I came up with several ways to get this to compile (let us use EventType as a type as well as hang property values off it). E.g.:

export interface EventType {}
export var EventType: {
  readonly COMPLETE: EventType
}

or:

export class EventType {
  static readonly COMPLETE: EventType;
}

but they didn't provide type-safety, since TypeScript's structural typing would let you pass anything (e.g. {x: 1}) for a parameter typed as EventType. 😕

So the only thing I found that actually works for type-safety is using export enum EventType { COMPLETE } like you originally had. So if you feel like putting that back, I would be okay with that. Else if you're tired of messing with it, this is okay too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. My "I couldn't get it to work" response above is based on the current version that used the variable declaration w/ nested property syntax. I am inclined to leave that as is for now, as it matches the underlying library closer than the Enum values.

fn: (param?: T) => void
): void => {
// TODO(dimond): closure typing seems broken because WebChannel does
// not implement goog.events.Listenable
channel.listen(type, (param?: T) => {
channel.listen(type, (param: unknown) => {
try {
fn(param);
fn(param as T);
} catch (e) {
setTimeout(() => {
throw e;
Expand Down Expand Up @@ -371,10 +369,10 @@ export class WebChannelConnection implements Connection {
// compatible with the bug we need to check either condition. The latter
// can be removed once the fix has been rolled out.
// Use any because msgData.error is not typed.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const msgDataAsAny: any = msgData;
const msgDataOrError: WebChannelError | object = msgData;
const error =
msgDataAsAny.error || (msgDataAsAny[0] && msgDataAsAny[0].error);
msgDataOrError.error ||
(msgDataOrError as WebChannelError[])[0]?.error;
if (error) {
log.debug(LOG_TAG, 'WebChannel received error:', error);
// error.status will be a string like 'OK' or 'NOT_FOUND'.
Expand Down
85 changes: 72 additions & 13 deletions packages/webchannel-wrapper/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,73 @@
* limitations under the License.
*/

// TODO(mikelehen): Flesh out proper types (or figure out how to generate via
// clutz or something).
export class XhrIo {}
export const ErrorCode: any;
export const EventType: any;
// WARNING: This is not a complete set of types exported by WebChannelWrapper.
// It is merely meant to support the usage patterns of the Firestore SDK.

export var EventType: {
COMPLETE: string;
};

export namespace WebChannel {
export type EventType = any;
export const EventType: any;
export var EventType: {
OPEN: string;
CLOSE: string;
ERROR: string;
MESSAGE: string;
};
}

export var ErrorCode: {
NO_ERROR: number;
HTTP_ERROR: number;
TIMEOUT: number;
};

export interface Headers {
[name: string]: string | number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why doesn't this cause problems like StringMap did?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted not to enable renaming for Object literal access. Since all headers are invalid JavaScript identifiers, they are only ever set using the literal access notation (such as header['X-Google-Foo'] = 42).

}

export interface WebChannelError {
error?: { status: string; message: string };
}

type StringMap = { [key: string]: string };
export class XhrIo {
send(
url: string,
method?: string,
body?: string,
headers?: Headers,
timeoutInterval?: number
): void;

getLastErrorCode(): number;

getLastError(): string;

getStatus(): number;

getResponseText(): string;

getResponseJson(): WebChannelError | object;

listenOnce(type: string, cb: (param: unknown) => void): void;
}

export interface WebChannelOptions {
messageHeaders?: StringMap;
initMessageHeaders?: StringMap;
messageHeaders?: {
// To ensure compatibility with property minifcation tools, keys need to
// be listed explicity.
[k: string]: never;
};
initMessageHeaders?: {
// To ensure compatibility with property minifcation tools, keys need to
// be listed explicity.
[k: string]: never;
};
messageContentType?: string;
messageUrlParams?: StringMap;
messageUrlParams?: {
database?: string;
};
clientProtocolHeaderRequired?: boolean;
concurrentRequestLimit?: number;
supportsCrossDomainXhr?: boolean;
Expand All @@ -44,13 +94,22 @@ export interface WebChannelOptions {
fastHandshake?: boolean;
disableRedac?: boolean;
clientProfile?: string;
internalChannelParams?: { [key: string]: boolean | number };
internalChannelParams?: {
forwardChannelRequestTimeoutMs?: number;
};
xmlHttpFactory?: unknown;
requestRefreshThresholds?: { [key: string]: number };
}

export interface WebChannel {
open(): void;
close(): void;
listen(type: string, cb: (param: unknown) => void): void;
send(msg: unknown): void;
}

export interface WebChannelTransport {
createWebChannel(url: string, options: WebChannelOptions): any;
createWebChannel(url: string, options: WebChannelOptions): WebChannel;
}

export function createWebChannelTransport(): WebChannelTransport;
2 changes: 0 additions & 2 deletions packages/webchannel-wrapper/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,6 @@ goog.net.XhrIo.prototype['getLastError'] =
goog.net.XhrIo.prototype['getLastErrorCode'] =
goog.net.XhrIo.prototype.getLastErrorCode;
goog.net.XhrIo.prototype['getStatus'] = goog.net.XhrIo.prototype.getStatus;
goog.net.XhrIo.prototype['getStatusText'] =
goog.net.XhrIo.prototype.getStatusText;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used by our code. Removing it is technically a breaking change though for consumers of this package. We might be ok though: https://github.com/search?l=JavaScript&q=%22%40firebase%2Fwebchannel-wrapper%22&type=Code

goog.net.XhrIo.prototype['getResponseJson'] =
goog.net.XhrIo.prototype.getResponseJson;
goog.net.XhrIo.prototype['getResponseText'] =
Expand Down