-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add common appUtils to augment get_environment with app level data #9236
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
base: master
Are you sure you want to change the base?
Conversation
e951142
to
d75b0a7
Compare
0c515ca
to
a5bece4
Compare
return { | ||
platform: Platform.WEB, | ||
directory: path.dirname(packageJsonFile), | ||
frameworks: getFrameworksFromPackageJson(packageJson), |
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.
What about web? Do we want to call detectAppIdsForPlatform
here?
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 left web out because it's actually a little tricky. You can put the web configuration in any part of your code, so in the worst we would need to read through the entire code base. :/
If you've got ideas for how to do this more efficiently, I'm all ears!
Technically the same is true of flutter, but there is at least a convention around it.
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 at least left a comment to that effect in the code.
platform: Platform; | ||
directory: string; | ||
appId?: string; | ||
bundleId?: 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.
Can there ever be a case where an app directory found >1 appId
/ bundleId
?
If the underlying detectAppIdsForPlatform
could potentially return multiple? Maybe it's safer to include all responses in the App
struct.
We can log those anomaly at a higher level.
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 can and does happen. In this case, I create multiple app instances. They are unique by appId and bundleId but may have the same platform and directory.
2ea682b
to
2add8f1
Compare
${dump(getAllAccounts().map((account) => account.user.email)).trim()} | ||
Detected App IDs: ${detectedAppsMap.size > 0 ? `\n\n${dump(Object.fromEntries(detectedAppsMap)).trim()}\n` : "<NONE>"} | ||
Available Project Aliases (format: '[alias]: [projectId]'): ${Object.entries(rc.projects).length > 0 ? `\n\n${dump(rc.projects).trim()}\n` : "<NONE>"} | ||
Available Accounts: ${hasOtherAccounts ? `\n\n${dump(allAccounts).trim()}` : "<NONE>"} |
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 we omit this header entirely if there are no other accounts logged in? Its a pretty rare use case in the first place.
If this project does not use Firebase tools that require a firebase.json file, no action is necessary. | ||
It looks like the current directory is not initialized as a Firebase project. The user will most likely want to: | ||
If this project uses Firebase tools that require a firebase.json file, the user will most likely want 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.
If this project uses Firebase tools that require a firebase.json file, the user will most likely want to: | |
If this project uses Firebase services that require a firebase.json file, the user will most likely want to: |
: `\n# Empty Environment | ||
: `\nNo firebase.json file was found. | ||
If this project does not use Firebase tools that require a firebase.json file, no action is necessary. |
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.
If this project does not use Firebase tools that require a firebase.json file, no action is necessary. | |
If this project does not use Firebase services that require a firebase.json file, no action is necessary. |
const leanApp = Object.fromEntries( | ||
Object.entries(app).filter((entry) => entry[1] !== undefined), | ||
) as App; | ||
console.log(`LEAN APP: ${JSON.stringify(leanApp)}`); |
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.
Remove
App, | ||
} from "./appUtils"; | ||
|
||
const FLUTTER_CONFIG = ` |
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.
Rather than having these configs as constants and setting up new directories every time, we should just add the test directories to the src/test/fixtures directory
@@ -0,0 +1,314 @@ | |||
import * as fs from "fs-extra"; |
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 file seems to duplicate a lot of what we've built in https://github.com/firebase/firebase-tools/blob/master/src/dataconnect/appFinder.ts - we really ought to unify the 2. I recall @fredzqm was looking at doing something like this
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 have followup PR to do just that. I had some of that in here, but Fred recommended I take it out. See the conversation here: #9236 (comment)
Description
Scenarios Tested
Sample Commands