Skip to content

Type issue in 8.12 conflicts with firebase-functions #880

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

Closed
samtstern opened this issue May 7, 2020 · 5 comments · Fixed by #881
Closed

Type issue in 8.12 conflicts with firebase-functions #880

samtstern opened this issue May 7, 2020 · 5 comments · Fixed by #881
Assignees

Comments

@samtstern
Copy link
Contributor

[READ] Step 1: Are you in the right place?

👍

[REQUIRED] Step 2: Describe your environment

  • Operating System version: all
  • Firebase SDK version: 8.12
  • Firebase Product: auth
  • Node.js version: 8/10
  • NPM version: all

[REQUIRED] Step 3: Describe the problem

When upgrading to 8.12 from 8.11 my build failed with:

##[error]node_modules/firebase-functions/lib/providers/auth.d.ts(14,22): error TS2420: Class 'UserRecordMetadata' incorrectly implements interface 'UserMetadata'.
  Property 'lastRefreshTime' is missing in type 'UserRecordMetadata' but required in type 'UserMetadata'.

The relevant line in firebase-functions is here:
https://github.com/firebase/firebase-functions/blob/87e75d717d5acf32d893e867078c531542d30c69/src/providers/auth.ts#L55

It seems that lastRefreshTime was added here:
#726

Maybe it should be optional not string | null

Steps to reproduce:

firebase/snippets-node#105

Relevant Code:

@rsgowman
Copy link
Member

rsgowman commented May 7, 2020

Huh. Conceptually, I think this should be string | null as we always set it to one or the other. It's not any different from lastSignInTime or creationTime (except it allows the possibility of null.) It's always present. We could change that a bit in this particular case to an optional (non-nullable) string. (Absent would then imply null which seems close enough.)

Though the more general problem is what should happen when we change anything to one of our interfaces. I didn't realise that functions implemented auth's interfaces; it seems that just about any change would cause it to break? (Short of marking everything as optional, which seems slightly unfriendly to consumers of the interface.)

Possible solutions:

  1. string|null -> ?string. Works here, but not in the general case.
  2. T -> ?T for all T in all interfaces.
  3. Change functions to reflect the (new) interface.
  4. ?

I'll leave the decision to @hiranya911

@samtstern
Copy link
Contributor Author

I guess it depends on if we are intentionally exporting these interfaces. Also we might want to make a distinction on input types (that developers have to create) and export types (things we return).

Adding fields to the latter is not a big deal, adding fields to the former is breaking.

@hiranya911
Copy link
Contributor

UserMetadata is an output type, and therefore adding a new field to it should not be a breaking change. Or so we thought :)

But I suppose we cannot control how people use any of our interfaces, regardless of whether they are just output types or not. People might implement them for various reasons (testing, extending etc). So we basically cannot add/remove required fields in any of our existing interfaces.

@rsgowman can we go ahead and change this to ?: string|null? We should also get the Functions team to update their implementation regardless.

rsgowman added a commit that referenced this issue May 7, 2020
While it's always present (even if null), having it optional allows
existing code that implements this interface to not break.

Fixes: #880
rsgowman added a commit that referenced this issue May 7, 2020
While it's always present (even if null), having it optional allows
existing code that implements this interface to not break.

Fixes: #880
@mrgrauel
Copy link

mrgrauel commented May 8, 2020

Are you going to release a bug fix update?

@samtstern
Copy link
Contributor Author

@mrgrauel 8.12.1 has been released with a fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants