Refactor FirebaseError

[mmermerkaya]

• Fixes and improves types.
• Uses extends Error and super instead of messing with prototypes for inheritance.
• Fixes inheritance for ES5 using setPrototypeOf, as recommended by TS.
• Displays a console warning if an entry from error.data overwrites a FirebaseError property.
• Removes the ability to define a custom pattern for template replacement, which should make packages more consistent. Nobody was actually using that feature anyway.
• Removes patchCapture. We run our tests in many browsers, including IE11 and Edge, so there is no need to fake remove captureStackTrace.
• Renames ErrorList to ErrorMap.
• Uses the actual ErrorFactory types in app-types package.

[Feiyang1]

Overall it looks really good to me. I think the size will be reduced, which will help reduce the size for the performance standalone script.

I will only push back on point 4 (Puts custom error data under error.data). It's an API change, also a breaking change which needs proper proposal and approval.

There is a cross platform proposal for standardizing error handling (error codes and error object structure) in go/firebase-error-handling. I'd like to look at error handling for JS SDK more holistically and make a proposal to address point 4 and other issues.

[Feiyang1]

I generally avoid using != for null/undefined checks. It's obvious for us what it does, but may not be obvious or even surprising for people who's new to javascript.
I think the originally snippet looks fine with a default value, any reason you changed it this way?

[mmermerkaya]

I'm not sure who you mean by "people new to JS". Is it other Google engineers, or outside contributors?

If it's Google engineers, this is something that is explicitly allowed in the style guide, which they should read if they're new to TS. If it's outside contributors, I don't think we should focus on making our code easy to understand for people who are new to JS. We're not teaching JS, we're building a product. Plus, I'd say == equality and it's quirks is one of JS fundamentals, and it's really easy to find tables like this.

Anyway, I switched to != null because most of our SDKs (including util) don't have strictNullChecks on. That means stuff can be null without any compiler errors. Plus it's the same amount of code, and it's (marginally) safer, so I thought why not? 🙂

Looking at this again, I guess it's very unlikely that someone would pass in null explicitly for data, and if so, it would fail a few lines later anyway, so I'm removing that check. I'll keep value != null though since I do need to check there.

[Feiyang1]

I disagree with the style guide, but it's not a big deal and I'm okay with being consistent with it.
But I will state my counter points anyway :)

1. My original point - Using !== makes it easier to understand. Removing the need/confusion of dealing with javascript quirks is always a good thing.
2. Sometimes you might just want to check null, but not undefined. When I review the code, if you used !=, I will always ask the question - Do you mean to check both null and undefined, or you actually want to only check null, but made a mistake. Being explicit can eliminate this confusion.

In this case, I can actually infer that it's the correct thing to check both null and undefined, so using != is probably fine here, but it could be harder if there are more complicated code to read.

[merlinnot]

There are some people who write JavaScript and TypeScript on a daily basis and don't know the quirks of loose equality comparison. I'm a contributor to this library. I'd honestly find it difficult to recall what this comparison does exactly.

I know many people who don't use it for long years now, as a principle. These people, including myself, avoid it like the plague, the same way as any in TypeScript.

Is the following reasonable? '\n 123 \t' == 123 // true 😋

[mmermerkaya]

@Feiyang1 Honestly, I disagree with the style guide too sometimes, like it allows single line if statements without {} brackets, which sounds very wrong to me 😄. I love consistency though, and overall it's a pretty good guide.

I agree with using === undefined (or === null) if you don't need to check both BTW, I have been doing that in my recent SDK code as well. But I'd rather play it safe in packages where the strict flag isn't turned on, and I prefer doing == null instead of === null || === undefined, maybe because as I'm already used to it, it doesn't look confusing to me anymore.

@merlinnot That looks horrible 😄.

I didn't mean that remembering the quirks is fundamental, I actually did horribly in this game a few months ago, most likely because I almost always avoid ==, just like you mentioned. I meant that almost everybody knows that == is weird, and if you know that, you can just look up what == null does in a few seconds. And doing == null to check both null and undefined is the only exception to the == ban in our style guide, so it's easy enough to remember after you see it a few times.

And any is the worst indeed 😄.

[Feiyang1]

Nice. Interesting that string enum satisfies the type constraint.

[Feiyang1]

Can you please apply this type across the repo?

[mmermerkaya]

What do you mean by apply this type? This should be compatible with the previous version, and the compiler isn't complaining, so I don't think we need to change anything anywhere else?

[Feiyang1]

For example, https://github.com/firebase/firebase-js-sdk/blob/master/packages/app/src/errors.ts#L29, we can use this new type instead.

[mmermerkaya]

Ah I see, sure, done 🙂

Looks like ErrorFactory is only used in App and Messaging 🤔

[mmermerkaya]

I removed the default string from ErrorList, so if you haven't typed your errors with an enum or a union type, you'd have to use ErrorList like ErrorList<string> explicitly, instead of just ErrorList. This doesn't break anything as ErrorList wasn't being used anywhere.