Remove Storage AuthWrapper layer #3399
Conversation
💥 No ChangesetLatest commit: f489262 Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Binary Size ReportAffected SDKs
Test Logs
|
| @@ -42,15 +38,14 @@ import * as errorsExports from './implementation/error'; | |||
| */ | |||
| export class StorageService { | |||
| private app_: FirebaseApp | null; | |||
There was a problem hiding this comment.
This can be readonly as well.
There was a problem hiding this comment.
It seems like it can't because deleteApp() sets it to null.
| @@ -14,6 +33,13 @@ module.exports = { | |||
| varsIgnorePattern: '^_', | |||
| args: 'none' | |||
| } | |||
| ], | |||
| 'import/no-extraneous-dependencies': [ | |||
There was a problem hiding this comment.
Are we using some devDependencies declared in the root package.json? What package was the offender? Should it be a dependency of @firebase/storage?
There was a problem hiding this comment.
We're not, accidental copy-paste artifact. I only put this here because vscode for some reason doesn't process the top-level packageDir path correctly, only if it's in the package's local eslintrc.js file.
There was a problem hiding this comment.
Do you mean we need it here to make the eslint plugin happy?
packages/storage/src/service.ts
Outdated
| this.app_ = app; | ||
| this.authProvider_ = authProvider; | ||
| this.requestMaker_ = makeRequest; |
There was a problem hiding this comment.
It seems unnecessary to first assign it to this.requestMaker_ since makeRequest is a pure function, unless we want to support multiple implementations of makeRequest, but I don't think it is the case?
There was a problem hiding this comment.
Yes, makes sense. Just leftover from AuthWrapper.
Remove
AuthWrapperclass wrapping Storage service. It no longer seems to be necessary and will simplify the code in preparation for modularization.