rewrite open command to typescript
#1403
Conversation
src/commands/open.ts
Outdated
| } else if (link.arg === "hosting:site") { | ||
| url = utils.addSubdomain(api.hostingOrigin, options.instance); | ||
| } else if (link.arg === "functions") { | ||
| url = "https://console.firebase.google.com/project/" + options.project + "/functions/list"; |
There was a problem hiding this comment.
I'd move the url without the path as a constant at the top like
const FIREBASE_CONSOLE = "https://console.firebase.google.com" then use format string here to make paths look nicer.
Also noticing...couldn't this be utils.consoleUrl(options.project, "/functions/list");?
There was a problem hiding this comment.
This particular one has been removed in favor of utils.consoleUrl.
src/commands/open.ts
Outdated
| url = "https://console.firebase.google.com/project/" + options.project + "/functions/list"; | ||
| } else if (link.arg === "functions:log") { | ||
| url = | ||
| "https://console.developers.google.com/logs/viewer?resource=cloudfunctions.googleapis.com&project=" + |
There was a problem hiding this comment.
Same as above, move to const at the top of the file as GCP_CONSOLE = ...
There was a problem hiding this comment.
I weigh these in my head: moving a string to a constant elsewhere in a file vs. keeping context where it's necessary. In this case, keeping the context well out-weighs moving part of this to a constant. It's only in one spot and it's important to realize this goes very much elsewhere.
That said, isn't this a really, really outdated URL (should it be console.cloud.google.com)? And why shouldn't it be utils.consoleUrl(options.project, "/functions/logs")?
There was a problem hiding this comment.
Yeah, I agree if it's not used anywhere else that it's fine to keep the URL here. Interesting point, this was probably going to the google cloud console because we didn't have logs implemented in firebase console? We should change that to open the Firebase console :)
There was a problem hiding this comment.
Okay. I'm fine with changing that, but I think that should be in a different CL since that's changing some behavior (I'd like to keep this having the same behavior... plus that little fix)
Description
Rewrite
opencommand to Typescript.Scenarios Tested
firebase openfirebase open hosting:sitefirebase open asdf(for the error)This fixes an internal bug: b/135410470