Make this work in Firebase Studio #1077
Conversation
joehan
changed the title
src/components/common/useRequest.ts
Outdated
| @@ -53,3 +55,7 @@ export function useRequest<T = unknown>( | |||
| refreshWhenOffline: true, | |||
| }); | |||
| } | |||
|
|
|||
| 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
| @@ -42,7 +42,7 @@ | |||
| "classnames": "^2.2.6", | |||
| "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
| result[key as Emulator] = { | ||
| ...value, | ||
| host, | ||
| port, | ||
| hostAndPort: hostAndPort(host, port), | ||
| }; | ||
| } | ||
|
|
||
| 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
| @@ -115,3 +115,23 @@ function toFormData(file: File) { | |||
| formData.append('upload', file); | |||
| return formData; | |||
| } | |||
|
|
|||
| 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 [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 <[email protected]>
Bunch of changes to get the emulator UI working in Firebase Studio:
While we're here, also doing some overdue maintainence: