Make run local always present #8024
Conversation
hlshen
force-pushed
the
hlshen/alwayslocal
branch
from
bca7b5a to
c86499c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8024 +/- ##
=======================================
Coverage 51.12% 51.13%
=======================================
Files 423 423
Lines 29722 29722
Branches 6083 6083
=======================================
+ Hits 15196 15198 +2
+ Misses 13160 13159 -1
+ Partials 1366 1365 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you! Great step to improve discoverability of "run local"!
[Not blocking] Would leave it up to you~
I see this PR adds a warning to ask if user wants to start the emulator.
I personally prefer to skip this question. The intent of "run local" is to run against the emulator, already super clear. Starting local emulator won't break anything. This question will add some frictions and increase drop-off from init local workspace to execute first operation.
I think we can show a warning that says "Automatically started the emulator. Please retry "run local" after it's started."
| const startEmulatorsCommand = vscode.commands.registerCommand( | ||
| "firebase.emulators.start", | ||
| startEmulatorsTask, | ||
| ); |
There was a problem hiding this comment.
Out of curiosity while I try to understand this code.
Excuse my ignorance. Very new to VS Code codebase.
What's the difference between broker and vscode.commands?
My guess is broker communicated any events between UI and extension process, so UI is never blocking. vscode.commands seem like something that can be triggered asynchronously from code, but so can broken.send
There was a problem hiding this comment.
As you said, broker is used specifically for communicating between Webviews and the extension process. The Webviews have no concept of "VSCode", so they would not be able to call vscode.comamnds directly.
| runTerminalTask( | ||
| "firebase emulators", | ||
| cmd, | ||
| { focus: true }, | ||
| ); | ||
| }; | ||
| const startEmulatorsTaskBroker = broker.on("runStartEmulators", () => { |
There was a problem hiding this comment.
I saw broker.on("runStartEmulators") twice in the code base.
Here and in emulator.ts. Would it make sense to dedup them?
Move to emulator.ts and re-use startEmulators method?
|
|
||
| // If the user selects "always", we update User settings. | ||
| if (result === always) { | ||
| configs.update(alwaysStartEmulator, true, ConfigurationTarget.Global); |
There was a problem hiding this comment.
If customer choose alwaysStartEmulator option, it sounds like we shouldn't show the above prompt again.
Shall we add a branch to skip it?
There was a problem hiding this comment.
Good catch! I should add a test for that too. I somehow missed that logic.
Run local will start the emulator if user agrees.
Follow up feature needed is executing the original request once emulator is fully started.