docs: add typings, plus new documentation, on the on() methods of Subscriber by feywind · Pull Request #1225 · googleapis/nodejs-pubsub

feywind · 2021-03-09T23:14:31Z

This adds some (hopefully optional) typings for the subscriber.on() method, as well as some JSDocs about the parameter types. I need to get some double-checking that this isn't going to end up being a breaking change, which I'm a bit nervous about after having to fix the implicit-any in subscription.ts.

  private _listen(): void {
    this.on('newListener', event => {
      if (!this.isOpen && event === 'message') {

If that ends up being a problem, we can also just comment out the typings for now. They're mostly for IDE support anyway.

I don't think there's an external issue for this, but it's based on b/74085082 (user report).

…ds of Subscriber

codecov · 2021-03-09T23:15:55Z

Codecov Report

Merging #1225 (3fd4043) into master (8838288) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1225   +/-   ##
=======================================
  Coverage   97.79%   97.80%           
=======================================
  Files          26       26           
  Lines       12607    12642   +35     
  Branches      611      562   -49     
=======================================
+ Hits        12329    12364   +35     
  Misses        273      273           
  Partials        5        5

Impacted Files	Coverage Δ
src/subscription.ts	`99.31% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8838288...3fd4043. Read the comment docs.

JustinBeckwith · 2021-03-09T23:20:35Z

+// not-quite-matching parameters won't cause a breaking change. We
+// may still shift these to required later.
+export declare interface Subscription {
+  on(event: 'message', listener: Function | ((message: Message) => void)): this;


The use of Function | <blergh> to me here is strange, because you're providing a nice function type for the user, and then making it more broadly just "Function". Can we get rid of the Function | piece, and always keep stricter bindings?

Mentioned on chat, but bringing it here for reference: without the Function it becomes a compile time breaking change for TypeScript users, because they may be passing in valid (but non-matching) callback handlers. For example, you could have a unit test that just passes in resolve to on(), and that would work before, but may not work now. My intent here is basically to give IDEs a breadcrumb trail to show the user the types, without breaking any existing code.

Ah, in that case - I would still drop Function (I think) and move to (message?: Message) => void so the parameter is optional. Would that get us where we want to go?

As a user I would expect the types to show whats possible in the api, optional when in fact it is not, would just confuse me. If its possible that on('message') will call my handler without a message parameter than the optional should be there to reflect that, on the other hand if there is no such case, then optional would make me think I have to handle a case that is not possible.
e.g: how it is done for node process is very clear
Test use-cases I would not consider for the legit types. Promise resolve is not going to break with a message, but in tests also any stub should work

my 2 cents from the user perspective, hope it helps :)
edit: I understand this might have to be a breaking change, but from my perspective it is still easier to change sooner than later.

@ettancos The perspective is much appreciated :) Maybe ((message: Message) => void) | (() => void) is clearer? I think that more or less works out to the same thing.

JustinBeckwith · 2021-03-09T23:21:36Z

   */
  private _listen(): void {
-    this.on('newListener', event => {
+    this.on('newListener', (event: string) => {


This was my hint above that something was amiss. I would expect the types to just figure out this is a string.

Yeah. Previously, on() was basically typed as Ben suggested below, since it's inherited from EventEmitter.

Still don't think we want to require the typing here.

bcoe · 2021-03-09T23:25:17Z

+  on(event: 'close', listener: Function): this;
+
+  // Only used internally.
+  on(event: 'newListener', listener: Function): this;


you could tighten this up more and declare the function signature:

(event: string): void

I could combine a few of these, like:

on(event: 'close'|'newListener'|etc, listener: Function): this;

But in general they need to be broken out, since my intent is to show the available events and what their parameters are.

I added a catch-all at the bottom that's impossible to match, to give somewhat better error messages. Otherwise the errors were all about on('removeListener') with it being at the bottom.

… but sure

…m-test that was broken by it

JustinBeckwith

Happy to hop on a quick chat if you want to tighten the loop on communication for this one, sorry for the delay.

JustinBeckwith · 2021-03-19T17:43:24Z

+export declare interface Subscription {
+  on(
+    event: 'message',
+    listener: (() => void) | ((message: Message) => void)


You could simplify this by collapsing to: listener: (message?: Message) => void

Here and below

Yeah, I did this in response to @ettancos 's comment. It's sort of misleading to say that Message is optional here, because it's not. It feels like a weird hack to get around a TS limitation.

At this point I think I'm prepared to just comment out the typings part for now, and re-add them on the next major. Getting the JSDoc in place is more immediately useful, and I think all of us are nervous about breaking existing clients with typings (for various reasons).

JustinBeckwith · 2021-03-19T17:45:13Z

+
+  // Catch-all. If you get an error about this line, it means you're
+  // using an unsupported event type or listener type.
+  on(event: string, listener: void): this;


I don't fully understand why this is needed. Are you saying that without it, using a non-enumerated event name doesn't cause a compile time error?

TS produces unhelpful error messages otherwise, if you use a non-enumerated event, or use a bad type for the callback on an enumerated one. It'll say that your call to on() doesn't match the definition for whatever on('foo') is declared last.

JustinBeckwith · 2021-03-19T17:45:47Z

   */
  private _listen(): void {
-    this.on('newListener', event => {
+    this.on('newListener', (event: string) => {


Still don't think we want to require the typing here.

…g change to the listener event handler

…to on-docs

* Move obtainTestInstance into the before block for backups * Extend expiry time * Revert "Extend expiry time" This reverts commit 8d42852803960f022bb9c6001cc247657b4c4433. * Revert "Move obtainTestInstance into the before block for" This reverts commit 3eb0d8fd8ed1ede77441e918f68a8f4d19e14a8f. * Add comment for backups * Add comments to functions file * Move obtainTestInstance in write module * Fix reads sample tests * Comment adjustment

docs: add optional typings, plus new documentation, on the on() metho…

acfacba

…ds of Subscriber

feywind requested review from a team March 9, 2021 23:14

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 9, 2021

product-auto-label Bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Mar 9, 2021

JustinBeckwith reviewed Mar 9, 2021

View reviewed changes

bcoe reviewed Mar 9, 2021

View reviewed changes

feywind added 3 commits March 16, 2021 16:53

build: i'm not sure why it keeps wanting to shift these lines around,…

0223147

… but sure

docs: update typings to be a bit more specific again, and fix a syste…

44e472f

…m-test that was broken by it

Merge remote-tracking branch 'remotes/origin/master' into on-docs

1757bbb

feywind changed the title ~~docs: add optional typings, plus new documentation, on the on() methods of Subscriber~~ docs: add typings, plus new documentation, on the on() methods of Subscriber Mar 16, 2021

feywind commented Mar 16, 2021

View reviewed changes

Comment thread samples/package.json

Merge branch 'master' into on-docs

1896d70

JustinBeckwith reviewed Mar 19, 2021

View reviewed changes

feywind added 2 commits March 19, 2021 13:59

fix: deactive typings for Subscription.on() for now, and revert typin…

e088375

…g change to the listener event handler

Merge branch 'on-docs' of https://github.com/feywind/nodejs-pubsub in…

3fd4043

…to on-docs

JustinBeckwith approved these changes Mar 19, 2021

View reviewed changes

feywind mentioned this pull request Mar 19, 2021

Enable typings for Subscriber.on() on the next major #1239

Closed

feywind merged commit 77be4b3 into googleapis:master Mar 19, 2021

feywind deleted the on-docs branch March 19, 2021 18:11

Conversation

feywind commented Mar 9, 2021

Uh oh!

codecov Bot commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

ettancos Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

JustinBeckwith left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Mar 9, 2021 •

edited

Loading

ettancos Mar 12, 2021 •

edited

Loading