RTDB accepts multi region URL #3050
Conversation
Binary Size ReportAffected SDKs
Test Logs |
| @@ -72,7 +72,7 @@ export const parseRepoInfo = function( | |||
| const parsedUrl = parseDatabaseURL(dataURL), | |||
| namespace = parsedUrl.namespace; | |||
|
|
|||
| if (parsedUrl.domain === 'firebase') { | |||
| if (parsedUrl.domain === 'firebase.com') { | |||
There was a problem hiding this comment.
Isn't this a breaking change for users that use firebase.com?ns=foo?
There was a problem hiding this comment.
Never mind. This is only breaking for firebase/ns?foo which shouldn't affect anyone.
There was a problem hiding this comment.
I believe in the past. *.firebase.* will hit this.
My guess is that RTDB was served in firebase.com instead of firebaseio.com in the past.
Added a test for this.
| @@ -174,17 +174,17 @@ export const parseDatabaseURL = function( | |||
| colonInd = dataURL.length; | |||
| } | |||
|
|
|||
| const parts = host.split('.'); | |||
| if (parts.length === 3) { | |||
| const hostWithoutPort = host.slice(0, colonInd); | |||
There was a problem hiding this comment.
From the slice docs:
"A negative index can be used, indicating an offset from the end of the sequence. slice(2,-1) extracts the third element through the second-to-last element in the sequence."
So, in this case, if no port is specified it seems like we would drop the last character, no?
There was a problem hiding this comment.
If no port is specified, colonInd = host.length above, so host.slice(0, host.length) ==> host
| } else { | ||
| const dotInd = host.indexOf('.'); | ||
| domain = host.substring(dotInd + 1); | ||
| subdomain = host.substring(0, dotInd).toLowerCase(); |
There was a problem hiding this comment.
Nit: Flip two lines above to parse string consecutively.
| @@ -62,6 +62,15 @@ describe('Database Tests', () => { | |||
| expect(db.ref().toString()).to.equal('https://foo.bar.com/'); | |||
| }); | |||
|
|
|||
| it('Can get database with multi-region URL', () => { | |||
There was a problem hiding this comment.
Do you want to add a test with a non-lowercase namespace?
There was a problem hiding this comment.
Sounds like a good idea.
There was a problem hiding this comment.
LGTM. Thanks for the new tests.
Can you add an entry to the Changelog? https://github.com/firebase/firebase-js-sdk/blob/master/packages/database/CHANGELOG.md
|
Done |
Allow SDK to parse url like
my-db.euw1.firebasedatabase.app