Add protocol as a separate property for FirebaseStorage service #5453
Conversation
hsubox76
requested review from
Feiyang1,
hiranya911 and
schmidt-sebastian
as code owners
🦋 Changeset detectedLatest commit: 307f7e3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
Changeset File Check ✅
|
| 'http://localhost:3001/v0/b/abcdefg.appspot.com/o/abcde.txt', | ||
| 'localhost:3001' | ||
| ); | ||
| expect(loc.bucket).to.equal('abcdefg.appspot.com'); |
There was a problem hiding this comment.
Should we test the protocol here as well?
There was a problem hiding this comment.
Location doesn't seem to store the host or the protocol, makeFromUrl just uses them to do regex matches and then throws them away. It only seems to have bucket, and path (just the part after the bucket).
packages/storage/src/service.ts
Outdated
| */ | ||
| private _host: string = DEFAULT_HOST; | ||
| protocol: string = 'https'; |
There was a problem hiding this comment.
Does this need to be exposed via the API?
There was a problem hiding this comment.
This whole class is internal so it shouldn't be exposed publicly. But it can't be private since connectStorageEmulator needs to be able to set it. I can add an underscore so it's clear that it's not meant to be public?
There was a problem hiding this comment.
The rest of the class does use underscores. I merely asked because of the change in the API report as it is not obvious that this is not exposed.
There was a problem hiding this comment.
Added an underscore.
packages/storage/src/service.ts
Outdated
| */ | ||
| private _host: string = DEFAULT_HOST; | ||
| protocol: string = 'https'; |
There was a problem hiding this comment.
The rest of the class does use underscores. I merely asked because of the change in the API report as it is not obvious that this is not exposed.
| 'http://localhost:3001/v0/b/abcdefg.appspot.com/o/abcde.txt', | ||
| 'localhost:3001' | ||
| ); | ||
| expect(loc.bucket).to.equal('abcdefg.appspot.com'); |
|
please add changeset |
I tried to avoid adding another property to the service in the initial emulator PR by having some logic in
makeUrlto deal with the fact thathostcould contain a protocol or not. UnfortunatelyLocation.refFromUrldoesn't usemakeUrland this caused a bug where there were duplicate protocol strings in the regex. Trying to keep "host" in a consistent format now to avoid future mistakes, separating "protocol" into a separate field.Fixes firebase/firebase-tools#3715