Export StorageError class instead of interface #6974
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: f7dd97d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
requested review from
dwyfrequency and
hsubox76
as code owners
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
|
I'm generally okay with this, is this consistent with other Firebase sdks? |
Yes. Every other SDK (except Auth) uses Classes instead of interfaces |
tonyjhuang
approved these changes
Jan 24, 2023
…base/firebase-js-sdk into mtewani/export-storageerror
tonyjhuang
approved these changes
Jan 25, 2023
hsubox76
reviewed
Jan 25, 2023
packages/storage/src/public-types.ts
Outdated
| import { CompleteFn, NextFn, Subscribe, Unsubscribe } from '@firebase/util'; | ||
| import { StorageError } from './implementation/error'; | ||
|
|
||
| export { StorageError, StorageErrorCode } from './implementation/error'; |
There was a problem hiding this comment.
I think this should be exported directly from src/api.ts. I think we had some weird bugs before when non-types were exported from a file that otherwise only exported types, but even if that's not a problem, just for organizational purposes.
hsubox76
approved these changes
Jan 25, 2023
common/api-review/storage.api.md
Outdated
| } | ||
|
|
||
| // @public | ||
| export const enum StorageErrorCode { |
There was a problem hiding this comment.
Sorry, wasn't paying attention earlier to a new enum being exported as well, I think there are some problems with exporting a const enum (for a certain set of users) so I think this needs to be changed to just enum here
Background on const enum issues https://ncjamieson.com/dont-export-const-enums/
hsubox76
reviewed
Jan 25, 2023
packages/storage-types/index.d.ts
Outdated
| @@ -16,10 +16,10 @@ | |||
| */ | |||
|
|
|||
| import { FirebaseApp } from '@firebase/app-types'; | |||
| import { StorageError as FirebaseStorageError } from '@firebase/storage'; | |||
There was a problem hiding this comment.
I'm not sure if this will work, as (1) @firebase/storage isn't a dependency of @firebase/storage-types so it's not great to import from it. In practice it should always be there because users install all of Firebase at once, but it doesn't seem right and I think some of the newer package managers yell about this. (2) I think this is sort of exporting a class from a d.ts file which I don't think you can do, not 100% sure, maybe it just treats it as a type. I know we were importing FirebaseError before which is a class but it was only used as an extends for an interface, and that interface was what's exported. Hm, is there any way to make an interface from scratch here that matches the StorageError class?
hsubox76
reviewed
Jan 26, 2023
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6944