Conversation
joehan
changed the title
src/components/common/useRequest.ts
Outdated
| } | ||
|
|
||
| function isCloudWorkstation(url: string) { | ||
| return url.includes('cloudworkstations.dev'); |
There was a problem hiding this comment.
I think this should be .endsWith
| return url.includes('cloudworkstations.dev'); | |
| return url.endsWith('.cloudworkstations.dev'); |
There was a problem hiding this comment.
Nope, url can include paths - ie: '9099-blah.cluster.cloudworkstations.dev/auth'
There was a problem hiding this comment.
nit: I think my preference is that we call this with the hostname path if we have it and therefore have endsWith just so that it's a stronger validation
| const loadCloudWorkstationCookies = async (url?: string) => { | ||
| if (url && url.includes('cloudworkstations.dev')) { | ||
| try { | ||
| const res = await fetchMaybeWithCredentials(url); |
There was a problem hiding this comment.
Something I'm considering down the line is exponential backoff for the situation when the user hasn't started the emulator yet
package.json
Outdated
| "codemirror": "^5.61.1", | ||
| "express": "^4.17.1", | ||
| "firebase": "^10.7.1", | ||
| "firebase": "^11.6.1-firebase-studio-sdk-integration.226be0bb1", |
There was a problem hiding this comment.
Are we merging this for an official release?
There was a problem hiding this comment.
No, we'll wait for the real release - adding a note to the descriptio n
| }; | ||
| } | ||
|
|
||
| if (result.firestore?.webSocketPort) { |
There was a problem hiding this comment.
previously you only remapped the webSocketPort if it was set, do you need to preserve this if statement?
There was a problem hiding this comment.
I think it's likely irrelevant (webSocketPort is always provided), but added it back in
src/components/common/rest_api.ts
Outdated
| } | ||
|
|
||
| function isCloudWorkstation(url: string) { | ||
| return url.includes('cloudworkstations.dev'); |
There was a problem hiding this comment.
let's prepend with a "." so we don't accidentally catch other domains that just happen to end in this string, see @maneesht comment in the other implementation of this
src/components/common/rest_api.ts
Outdated
| opts = {}; | ||
| } | ||
| if (isCloudWorkstation(url)) { | ||
| opts.credentials = 'include'; |
There was a problem hiding this comment.
I think you can just do
| opts.credentials = 'include'; | |
| opts = {...opts, credentials: 'include'}; |
It will handle undefined without Line 130-132 and it avoids modifying the argument
src/firebase.ts
Outdated
| const projectId = conf.projectId; | ||
| const [app, setApp] = useState<FirebaseApp | undefined>(); | ||
|
|
||
| void Promise.all([ |
There was a problem hiding this comment.
nit: I think you can just void each without Promise.all
Co-authored-by: Maneesh Tewani <maneesht@users.noreply.github.com>
4 participants
Bunch of changes to get the emulator UI working in Firebase Studio:
While we're here, also doing some overdue maintainence: