Add core types & error map to auth-exp #2890

avolkovi · 2020-04-09T17:28:26Z

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

Read the contribution guidelines (CONTRIBUTING.md).
If this has been discussed in an issue, make sure to link to the issue here.
If not, go file an issue about this before creating a pull request to discuss.

Testing

Make sure all existing tests in the repository pass after your change.
If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

At this time we cannot accept changes that affect the public API. If you'd like to help
us make Firebase APIs better, please propose your change in an issue so that we
can discuss it together.

google-oss-bot · 2020-04-09T17:37:41Z

Binary Size Report

Affected SDKs

No changes between base commit (85e6b06) and head commit (ff9ac63).

Test Logs

Base (85e6b063): https://github.com/firebase/firebase-js-sdk/actions/runs/73916987
Head (ff9ac63a): https://github.com/firebase/firebase-js-sdk/actions/runs/74716220

avolkovi · 2020-04-09T17:37:43Z

packages-exp/auth-exp/src/core/errors.ts

+  WEB_STORAGE_UNSUPPORTED = 'web-storage-unsupported'
+}
+
+const ERRORS: ErrorMap<AuthErrorCode> = {


I'll start the discussion here - is there a way we can make this optional? All these strings will be a large part of the compiled binary

As discussed offline, it might be good to have separate error maps for "dev" and "prod", that way the user could choose which gets included. That would require the AUTH_ERROR_FACTORY to not be a constant.

I think we can mull this for a while, but to make that change more straightforward in the future, maybe this module should export a function instead of a constant?

export function getErrorFactory() { return AUTH_ERROR_FACTORY }

interested in what others think about this... @bojeil-google ?

Just as a FYI, Firestore implemented something similar recently, but in a different way. They split their asserts into hardAsserts and debugAsserts, and debugAsserts are removed in the prod build.

#2859
#2814

I like your idea here that user could choose what errors to show.

The assert model is cool, but doesn't work great in auth since most of our error messages are translations for server side errors

Note that this list also includes client side errors that do not map to server side errors.
So you are suggesting to make error mapping functionality modular? So if the developer does not import it, then what happens?
Expecting the developer to know what errors to include puts a lot of burden on the developer. They would need to understand each error.

I think for production builds we could just have a short code that can be mapped in our documentation, something like Error: auth/email-exists, with the full description & troubleshooting tips available online

this would be a breaking API change though, and we would need the page up on the documentation before we could launch this, but it would be cool and could potentially make the final binary that much smaller

I think I misunderstood your suggestion. You are suggesting modularization of the error messages population component and not the entire client side errors (and how they are mapped from server to client). That may not be terrible but a breaking change, alas.

I agree we should optimize this but I would suggest we think about it more (this also seems like a general problem and not specific to Auth). JsCore should try to find a unified solution for it. I would recommend implementing the current spec in its current form for now and then experimenting with the generated binary and revisiting this to see how we can avoid bundling these messages.

agree on punting this to JS core for now, lets proceed as is, we can measure the possible effect of the strings during alpha and maybe implement this during beta

sam-gc · 2020-04-09T18:05:52Z

packages-exp/auth-exp/src/core/errors.ts

+  WEB_STORAGE_UNSUPPORTED = 'web-storage-unsupported'
+}
+
+const ERRORS: ErrorMap<AuthErrorCode> = {


As discussed offline, it might be good to have separate error maps for "dev" and "prod", that way the user could choose which gets included. That would require the AUTH_ERROR_FACTORY to not be a constant.

I think we can mull this for a while, but to make that change more straightforward in the future, maybe this module should export a function instead of a constant?

export function getErrorFactory() { return AUTH_ERROR_FACTORY }

packages-exp/auth-exp/src/model/auth.d.ts

packages-exp/auth-exp/src/model/user.ts

we can make an implementation class later

sam-gc · 2020-04-09T20:06:37Z

LGTM

* Add core types & error map * [AUTOMATED]: Prettier Code Styling * [AUTOMATED]: License Headers * User should be an interface for now we can make an implementation class later

avolkovi added 2 commits April 9, 2020 10:22

Add core types & error map

93d941e

[AUTOMATED]: Prettier Code Styling

b610f16

avolkovi requested review from Feiyang1, hiranya911 and hsubox76 as code owners April 9, 2020 17:28

avolkovi requested review from sam-gc and bojeil-google April 9, 2020 17:28

avolkovi assigned sam-gc Apr 9, 2020

[AUTOMATED]: License Headers

7b2c2ae

avolkovi commented Apr 9, 2020

View reviewed changes

sam-gc reviewed Apr 9, 2020

View reviewed changes

User should be an interface for now

76ed5f5

we can make an implementation class later

avolkovi merged commit a627ebf into auth-next Apr 10, 2020

avolkovi deleted the avolkovi/auth-next-api branch April 16, 2020 18:07

firebase locked and limited conversation to collaborators May 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add core types & error map to auth-exp #2890

Add core types & error map to auth-exp #2890

avolkovi commented Apr 9, 2020

google-oss-bot commented Apr 9, 2020 •

edited

Loading

avolkovi Apr 9, 2020

sam-gc Apr 9, 2020 •

edited

Loading

avolkovi Apr 9, 2020 •

edited

Loading

Feiyang1 Apr 9, 2020 •

edited

Loading

avolkovi Apr 9, 2020

bojeil-google Apr 9, 2020

avolkovi Apr 9, 2020

avolkovi Apr 9, 2020

bojeil-google Apr 10, 2020

avolkovi Apr 10, 2020

sam-gc Apr 9, 2020 •

edited

Loading

sam-gc commented Apr 9, 2020

Add core types & error map to auth-exp #2890

Add core types & error map to auth-exp #2890

Conversation

avolkovi commented Apr 9, 2020

Discussion

Testing

API Changes

google-oss-bot commented Apr 9, 2020 • edited Loading

Binary Size Report

Affected SDKs

Test Logs

avolkovi Apr 9, 2020

Choose a reason for hiding this comment

sam-gc Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

avolkovi Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Feiyang1 Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

avolkovi Apr 9, 2020

Choose a reason for hiding this comment

bojeil-google Apr 9, 2020

Choose a reason for hiding this comment

avolkovi Apr 9, 2020

Choose a reason for hiding this comment

avolkovi Apr 9, 2020

Choose a reason for hiding this comment

bojeil-google Apr 10, 2020

Choose a reason for hiding this comment

avolkovi Apr 10, 2020

Choose a reason for hiding this comment

sam-gc Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

sam-gc commented Apr 9, 2020

google-oss-bot commented Apr 9, 2020 •

edited

Loading

sam-gc Apr 9, 2020 •

edited

Loading

avolkovi Apr 9, 2020 •

edited

Loading

Feiyang1 Apr 9, 2020 •

edited

Loading

sam-gc Apr 9, 2020 •

edited

Loading