Add typings for WebChannel #2385

schmidt-sebastian · 2019-11-27T19:45:33Z

With inspiration from

https://github.com/fivetran/typescript-closure-tools/blob/master/index/closure-library/closure/goog/net/errorcode.d.ts
and https://github.com/fivetran/typescript-closure-tools/blob/master/index/closure-library/closure/goog/net/xhrio.d.ts

mikelehen

I think these types are diverging from our (unfortunate) reality. 😄

mikelehen · 2019-12-02T17:22:00Z

packages/webchannel-wrapper/src/index.d.ts

+    body?: ArrayBufferView | Blob | string | null,
+    headers?: Headers,
+    timeoutInterval?: number
+  ): Promise<XhrIo>;


Why does this return a Promise? Looks like void to me?

https://github.com/fivetran/typescript-closure-tools/blob/20d3931ebe25f1f73e77794d79c9cce786540502/index/closure-library/closure/goog/net/xhrio.d.ts#L90

My initial attempt to the types from Google3 is the source of this:

https://cs.corp.google.com/piper///depot/google3/third_party/javascript/firebase/src/packages/storage/src/implementation/xhrio.ts?q=%22Promise%3CXhrIo%3E%22&dr=C&l=26

This also explains retroactively why the input types are limited to ArrayBufferView | Blob | string.

mikelehen · 2019-12-02T17:26:14Z

packages/webchannel-wrapper/src/index.d.ts

+  send(
+    url: string,
+    method?: string,
+    body?: ArrayBufferView | Blob | string | null,


I don't care too much, but this doesn't exactly match the closure types (which include ArrayBuffer, Document, FormData, etc.) or our own usage (where we just pass string I assume).

If somebody ever needs to modify this in the future, it may not be clear how we ended up with the existing type definition and so it may not be clear how to properly update it.

My new goal is to only support the usage in Firestore. As such, I changed it to just support string.

mikelehen · 2019-12-02T19:03:05Z

packages/webchannel-wrapper/src/index.d.ts

+    CLOSE = 'close',
+    ERROR = 'error',
+    MESSAGE = 'message',
+    COMPLETE = 'complete'


complete doesn't seem to exist on webchannel or be used by our code?

Naive me thought it was possible to re-use the same EventType typings in two different places. I have since gone back on this approach and embraced reality.

mikelehen · 2019-12-02T19:22:11Z

packages/webchannel-wrapper/src/index.d.ts

+}
+
+export enum ErrorCode {
+  NO_ERROR = 0,


Again, this doesn't match reality (where we only export NO_ERROR, TIMEOUT, and HTTP_ERROR).

Yeah - I removed all not exported members. As part of this I also saw that we export getResponseText but don't use it. I removed it from the closure wrapper.

mikelehen · 2019-12-02T19:23:24Z

packages/webchannel-wrapper/src/index.d.ts

+
+  getResponseText(): string;
+
+  getResponseJson(): Object | any;


nit: is blah | any better / different than just any?

Nope. But my new way of returning WebChannelError might be (which I think is needed since we access error.code and error.message.

mikelehen · 2019-12-02T19:26:25Z

packages/webchannel-wrapper/src/index.d.ts

+  CLOSE = 'close',
+  ERROR = 'error',
+  MESSAGE = 'message',
+  COMPLETE = 'complete'


Hrm... I don't think this is right. Our webchannel-wrapper module actually only exports EventType.COMPLETE and the value should come from closure-library (goog.net.EventType.COMPLETE) rather than us redefine the value to a constant ('complete') here. See:

firebase-js-sdk/packages/webchannel-wrapper/src/index.js

Line 49 in a864185

goog.net.EventType['COMPLETE'] = goog.net.EventType.COMPLETE;

firebase-js-sdk/packages/webchannel-wrapper/src/index.js

Line 87 in a864185

'EventType': goog.net.EventType,

With your change, this .d.ts file will now let us use e.g. EventType.OPEN in our code but at runtime it will be undefined because the actual implementation (in webchannel-wrapper/index.js) doesn't actually define it.

So I think we need to reduce this to only what we export from index.js... and I don't think we should be assigning values ('open' etc.) to the enum values. In fact, maybe we shouldn't use an enum at all, to make it clearer that the values aren't known (to TypeScript) at compile-time. Poking around, I'm not sure what the right way to do that is, but this seems to work:

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

I wonder if there's some better way to go about all of this. webchannel-wrapper is an ugly wart to allow us to use (a very small subset of) closure-library from our TypeScript code. The fact that https://github.com/fivetran/typescript-closure-tools exists suggests that maybe there's a better way and we wouldn't have to manually re-expose the part of closure-library we need? Perhaps a good hackweek project! 😛

I should have done more research prior to this PR, but I think at least now I have gained a rough understanding of how the exports work. I traced them through to the minified code and replaced all types with strings (or numbers for ErrorCode).

mikelehen · 2019-12-02T20:50:42Z

packages/webchannel-wrapper/src/index.d.ts

@@ -31,7 +82,9 @@ export interface WebChannelOptions {
  messageHeaders?: StringMap;
  initMessageHeaders?: StringMap;


Are these usages of StringMap safe for some reason? I'm not clear why you had to change messageUrlParams and internalChannelParams but not these.

That said, it would be nice if you didn't have to change any of these and our minification logic was able to handle StringMap. Is that not possible?

They are only used as empty objects in Firestore. I ended up removing the string map altogether and changed the types to {}. I hope that may act as more of a deterrent.

mikelehen · 2019-12-02T20:51:48Z

packages/webchannel-wrapper/src/index.d.ts

+  close(): void;
+  halfClose(): void;
+  listen<T>(type: EventType, cb: (param: T) => void): void;
+  send<T>(msg: T): void;


nit: Is there a benefit to making these generic instead of typing them as unknown or similar?

Send can be unknown (it should probably not be - but then we should both the generic type on WebChannel). I made it unknown for now.
The in listen calls back to us so it cannot be unknown.

schmidt-sebastian · 2019-12-02T22:43:16Z

packages/webchannel-wrapper/src/index.d.ts

+    body?: ArrayBufferView | Blob | string | null,
+    headers?: Headers,
+    timeoutInterval?: number
+  ): Promise<XhrIo>;


My initial attempt to the types from Google3 is the source of this:

https://cs.corp.google.com/piper///depot/google3/third_party/javascript/firebase/src/packages/storage/src/implementation/xhrio.ts?q=%22Promise%3CXhrIo%3E%22&dr=C&l=26

This also explains retroactively why the input types are limited to ArrayBufferView | Blob | string.

schmidt-sebastian · 2019-12-02T22:46:01Z

packages/webchannel-wrapper/src/index.d.ts

+    CLOSE = 'close',
+    ERROR = 'error',
+    MESSAGE = 'message',
+    COMPLETE = 'complete'


Naive me thought it was possible to re-use the same EventType typings in two different places. I have since gone back on this approach and embraced reality.

schmidt-sebastian · 2019-12-02T22:48:11Z

packages/webchannel-wrapper/src/index.d.ts

+  CLOSE = 'close',
+  ERROR = 'error',
+  MESSAGE = 'message',
+  COMPLETE = 'complete'


I should have done more research prior to this PR, but I think at least now I have gained a rough understanding of how the exports work. I traced them through to the minified code and replaced all types with strings (or numbers for ErrorCode).

schmidt-sebastian · 2019-12-02T22:49:11Z

packages/webchannel-wrapper/src/index.d.ts

+}
+
+export enum ErrorCode {
+  NO_ERROR = 0,


Yeah - I removed all not exported members. As part of this I also saw that we export getResponseText but don't use it. I removed it from the closure wrapper.

schmidt-sebastian · 2019-12-02T22:50:24Z

packages/webchannel-wrapper/src/index.d.ts

+  send(
+    url: string,
+    method?: string,
+    body?: ArrayBufferView | Blob | string | null,


My new goal is to only support the usage in Firestore. As such, I changed it to just support string.

schmidt-sebastian · 2019-12-02T22:51:47Z

packages/webchannel-wrapper/src/index.d.ts

+
+  getResponseText(): string;
+
+  getResponseJson(): Object | any;


Nope. But my new way of returning WebChannelError might be (which I think is needed since we access error.code and error.message.

schmidt-sebastian · 2019-12-02T22:52:31Z

packages/webchannel-wrapper/src/index.d.ts

@@ -31,7 +82,9 @@ export interface WebChannelOptions {
  messageHeaders?: StringMap;
  initMessageHeaders?: StringMap;


They are only used as empty objects in Firestore. I ended up removing the string map altogether and changed the types to {}. I hope that may act as more of a deterrent.

schmidt-sebastian · 2019-12-02T23:16:44Z

packages/webchannel-wrapper/src/index.d.ts

+  close(): void;
+  halfClose(): void;
+  listen<T>(type: EventType, cb: (param: T) => void): void;
+  send<T>(msg: T): void;


Send can be unknown (it should probably not be - but then we should both the generic type on WebChannel). I made it unknown for now.
The in listen calls back to us so it cannot be unknown.

schmidt-sebastian · 2019-12-02T23:57:10Z

packages/webchannel-wrapper/src/index.js

@@ -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;


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

mikelehen · 2019-12-03T01:30:54Z

packages/firestore/src/platform_browser/webchannel_connection.ts

@@ -371,10 +369,9 @@ 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 msgDataAsAny: WebChannelError | object = msgData;


The name msgDataAsAny did not survive your refactor very well. msgDataOrError ?

Sounds good. Renamed.

mikelehen · 2019-12-03T01:33:19Z

packages/webchannel-wrapper/src/index.d.ts

+};
+
+export interface Headers {
+  [name: string]: string | number;


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

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).

mikelehen · 2019-12-03T01:36:11Z

packages/webchannel-wrapper/src/index.d.ts

-  messageHeaders?: StringMap;
-  initMessageHeaders?: StringMap;
+  messageHeaders?: {};
+  initMessageHeaders?: {};


Is your intention to require that we set these to an empty object? Because I don't think that's what this actually does. This will allow us to pass any object (e.g. {'foo': 'bar'} since TypeScript does structural typing (aka duck typing).

Not sure it's possible to specify a type that indicates that only an empty object is valid.

That said, in general this is feeling somewhat fragile which is a bit worrisome since webchannel is already a bit fragile. It wouldn't be too surprising if we tweak some webchannel setting for performance/reliability reasons and then it gets minified wrong because we don't properly update this d.ts file and therefore it's ignored, and we don't notice because it's not something we can readily test.

If that's the world we're heading towards, I'd like to discuss if there are ways to avoid it... and if not, then we will perhaps eventually (once your minification stuff lands) want some big warnings in this file or something.

To be perfectly honest, that was my intention. I did realize this doesn't work but I thought that it might still convey some of the original intent.

But you can ignore all this because I came up with a way that actually works. I can disallow all properties with:

messageHeaders: { [k : string] : never }

Furthermore, all members that have at least a single property specified do not allow any extra properties as is.

mikelehen · 2019-12-03T01:45:45Z

packages/webchannel-wrapper/src/index.d.ts

+export interface WebChannel {
+  open(): void;
+  close(): void;
+  listen<T>(type: string, cb: (param: T) => void): void;


I still don't really understand this usage of generics. Is there even any practical difference between:

function foo(x: any) // and function foo<T>(x: T)

It seems like it's a way of telling TypeScript to let you pass anything. The generic way looks more type-safe, but I don't think it is?

It seems like the correct type here is unknown and we should explicitly cast to something else if we need to.

No - there is no difference, but one looks safer! I changed this to unknown and modified the consumer.

mikelehen · 2019-12-03T01:45:58Z

packages/webchannel-wrapper/src/index.d.ts

+
+  getResponseJson(): WebChannelError | object;
+
+  listenOnce<T>(type: string, cb: (param: T) => void): void;


same feedback about generics here.

Removed. I am "casting" in the callsite now.

mikelehen · 2019-12-03T01:47:56Z

packages/firestore/src/platform_browser/webchannel_connection.ts

@@ -310,7 +308,7 @@ 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,


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.

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

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.

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.

…s-sdk into mrschmidt/webchannel

mikelehen · 2019-12-03T23:27:28Z

packages/firestore/src/platform_browser/webchannel_connection.ts

@@ -310,7 +308,7 @@ 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,


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.

schmidt-sebastian added the api: firestore label Nov 27, 2019

schmidt-sebastian force-pushed the mrschmidt/webchannel branch 2 times, most recently from 1cce950 to 9f7b146 Compare December 1, 2019 22:11

Add Types for Webchannel

a864185

schmidt-sebastian force-pushed the mrschmidt/webchannel branch from 9f7b146 to a864185 Compare December 2, 2019 04:59

schmidt-sebastian requested a review from mikelehen December 2, 2019 06:07

schmidt-sebastian assigned mikelehen Dec 2, 2019

mikelehen suggested changes Dec 2, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Dec 2, 2019

schmidt-sebastian added 2 commits December 2, 2019 15:17

Review comments

e9c7d01

[AUTOMATED]: Prettier Code Styling

82f6645

schmidt-sebastian commented Dec 2, 2019

View reviewed changes

Update index.d.ts

dc0b8a5

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Dec 2, 2019

mikelehen reviewed Dec 3, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Dec 3, 2019

schmidt-sebastian added 3 commits December 2, 2019 21:09

More review feedback

3f76270

[AUTOMATED]: Prettier Code Styling

2936fec

Merge branch 'mrschmidt/webchannel' of github.com:firebase/firebase-j…

1f4767b

…s-sdk into mrschmidt/webchannel

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Dec 3, 2019

mikelehen approved these changes Dec 3, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Dec 3, 2019

schmidt-sebastian merged commit 81730ba into master Dec 3, 2019

hsubox76 added this to the next milestone Dec 10, 2019

firebase locked and limited conversation to collaborators Jan 3, 2020

schmidt-sebastian deleted the mrschmidt/webchannel branch February 28, 2020 23:03

		@@ -31,7 +82,9 @@ export interface WebChannelOptions {
		messageHeaders?: StringMap;
		initMessageHeaders?: StringMap;


		getResponseJson(): WebChannelError \| object;

		listenOnce<T>(type: string, cb: (param: T) => void): void;

Add typings for WebChannel #2385

Add typings for WebChannel #2385

Uh oh!

Conversation

schmidt-sebastian commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Nov 27, 2019 •

edited

Loading