-
Notifications
You must be signed in to change notification settings - Fork 917
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support multiple accounts #3192
Conversation
@@ -1,50 +0,0 @@ | |||
"use strict"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-written in TS
Did some testing, and noticed that |
@joehan thanks! I just pushed a fix for this, I always forget about "inherited options" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some cleanup, but most of the core logic seems good to me
src/auth.ts
Outdated
|
||
// Clear any matching project defaults | ||
const activeAccounts = configstore.get("activeAccounts") || {}; | ||
for (const projectDir of Object.keys(activeAccounts)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer using for.. in here instead of Object.keys, unless theres something reason not to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it but then eslint yelled at me about guarding it, which I think would be uglier:
https://eslint.org/docs/rules/guard-for-in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, yeah, I ran into that same rule recently. Maybe we should get rid of the rule, but I agree this is less ugly than adding an eslint-ignore or guarding it
configstore.set("tokens", lastAccessToken); | ||
const account = findAccountByRefreshToken(refreshToken); | ||
if (account && lastAccessToken) { | ||
account.tokens = lastAccessToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Why is this account.tokens if it only has one token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to keep terminology similar where I can. The "tokens"
key has been used in configstore to refer to the object of type Tokens
which contains access and refresh tokens. This is the same type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once you fix the typo in commands/index.js
Probably deserves a second set of eyes though given the scope.
Co-authored-by: joehan <[email protected]>
@@ -0,0 +1 @@ | |||
- Adds support for multiple accounts via new commands `login:use`, `login:add` and `login:list`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a feature that could do with (a) documentation in the README and (b) documentation in FireSite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @rachelsaunders I added a README section, can you take a look?
* Get the default account associated with a project directory, or the global default. | ||
* @param projectDir the Firebase project directory. | ||
*/ | ||
export function getProjectDefaultAccount(projectDir?: string | null): Account | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can projectDir
really be null
? What does that mean rather than just an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from detectProjectRoot
which sets options.projectRoot
to string | null
... it's a nasty type but I want the code to be honest about what options.projectRoot
(which is typed as any
) can be and because detectProjectRoot
is not always called both undefined
and null
are possible.
/** | ||
* Get all authenticated accounts _besides_ the default account. | ||
*/ | ||
export function getAdditionalAccounts(): Account[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing that all these new functions are exported - is that necessary? Some of these seem like helpers...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're all used in at least one other file.
); | ||
} | ||
|
||
const projectDir = options.projectRoot as string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just options.projectRoot as string || ""
? (I'm not a fan of using null
if I can help it, as you may have guessed by now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual type based on detectProjectRoot
, the as
is just because I don't want to use any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't hammer every corner, I'm sure, but I pulled this down and played with it a bit, init-ing and deploying things. Looks good to me!
Description
Implement multiple authentication (see Internal API proposal)
Fixes #1906
Fixes #181
Scenarios Tested
login:add
login:list
login:use
logout
logout (primary account
init
--account flag (denied)
--account flag (succeed)
Sample Commands
Internal API proposal