Convert localDev and LocalDevManager to TS #1339
Conversation
camden11
requested review from
brandenrodgers,
kemmerle and
joe-yeager
as code owners
| export async function confirmDefaultAccountIsTarget( | ||
| accountConfig: CLIAccount | ||
| ): Promise<void> { | ||
| if (!accountConfig.name || !accountConfig.accountType) { |
There was a problem hiding this comment.
I'm a little wary of adding new logs in a TS port since it could subtly change the functionality. Would it be possible to break the code additions out into a standalone PR to merge and test before this one?
There was a problem hiding this comment.
Any new logs added here are more or less impossible to actually reach. I added them to satisfy TS (throwing errors makes typing a lot easier than having to detail with returning undefined). I guess theoretically this would trigger if you went in and manually removed these fields from the config but I have to imagine this would be better than whatever the behavior was previously. If you still think its worth separating out I can do that though
| @@ -205,11 +249,11 @@ class LocalDevManager { | |||
| getProjectDetailUrl( | |||
There was a problem hiding this comment.
I wonder why getProjectDetailUrl returns if the projectName doesn't exist. The projectName field is required
There was a problem hiding this comment.
I think that was left over from before it was typed. Once we're sure everywhere in the CLI is passing in a string, we can probably remove that check
Description and Context
This converts the last remaining files in
lib,localDevandlocalDevManagerto TSTODO
Still need to run through the local dev flows and make sure this doesn't break anythingIt works!Who to Notify
@brandenrodgers @kemmerle @joe-yeager