feat(cli): add --firebase-out flag for custom firebase.json path#435
feat(cli): add --firebase-out flag for custom firebase.json path#435kevmoo wants to merge 3 commits intoinvertase:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --firebase-out flag to the configure and reconfigure commands, allowing users to specify a custom path for the firebase.json file. The changes involve updating argument parsing, path resolution logic, and adding a corresponding test case. Reviewer feedback suggests that custom paths should be resolved relative to the project root to ensure consistent behavior across different execution environments.
|
/gemini review |
github-actions
Bot
added
the
Needs Attention
There was a problem hiding this comment.
Code Review
This pull request introduces a new --firebase-out flag to the configure and reconfigure commands, enabling users to specify a custom path for the firebase.json file. The path resolution logic is now centralized in the FlutterFireCommand base class, and the changes are verified with new integration tests. Feedback highlights the need for defensive programming to handle cases where the Flutter app context might be null and recommends consistent help text for the new flag across different commands.
| String resolveFirebaseJsonPath(String? customPath) { | ||
| if (customPath != null) { | ||
| // If a custom path was provided, check if it is relative. | ||
| if (path.isRelative(customPath)) { | ||
| // Resolve relative paths relative to the project root to ensure consistent behavior. | ||
| return path.join(flutterApp!.package.path, customPath); | ||
| } | ||
| // Use absolute paths as provided. | ||
| return customPath; | ||
| } | ||
| // Default to the standard firebase.json in the project root if no custom path is specified. | ||
| return path.join(flutterApp!.package.path, 'firebase.json'); | ||
| } |
There was a problem hiding this comment.
The resolveFirebaseJsonPath method uses the null-check operator (!) on flutterApp multiple times. While most commands in this CLI require a Flutter app, FlutterFireCommand allows flutterApp to be null. If this method is called when flutterApp is null, it will throw a runtime error. Consider adding a defensive check or using a fallback like Directory.current.path to determine the project root.
| String resolveFirebaseJsonPath(String? customPath) { | |
| if (customPath != null) { | |
| // If a custom path was provided, check if it is relative. | |
| if (path.isRelative(customPath)) { | |
| // Resolve relative paths relative to the project root to ensure consistent behavior. | |
| return path.join(flutterApp!.package.path, customPath); | |
| } | |
| // Use absolute paths as provided. | |
| return customPath; | |
| } | |
| // Default to the standard firebase.json in the project root if no custom path is specified. | |
| return path.join(flutterApp!.package.path, 'firebase.json'); | |
| } | |
| String resolveFirebaseJsonPath(String? customPath) { | |
| final projectRoot = flutterApp?.package.path ?? Directory.current.path; | |
| if (customPath != null) { | |
| // If a custom path was provided, check if it is relative. | |
| if (path.isRelative(customPath)) { | |
| // Resolve relative paths relative to the project root to ensure consistent behavior. | |
| return path.join(projectRoot, customPath); | |
| } | |
| // Use absolute paths as provided. | |
| return customPath; | |
| } | |
| // Default to the standard firebase.json in the project root if no custom path is specified. | |
| return path.join(projectRoot, 'firebase.json'); | |
| } |
There was a problem hiding this comment.
This might need further investigating.
|
"Hi! I noticed the CI tests are failing. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
russellwheatley
had a problem deploying
to
CI Environment
GitHub Actions
Failure
russellwheatley
had a problem deploying
to
CI Environment
GitHub Actions
Failure
russellwheatley
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Yep, I don't have an issue with this PR, FlutterFire CLI is meant to be highly configurable to an extent.
There's a CI test(s) failing for linux (probably macos once finished) that needs to be resolved. You also need to update the README.md documentation with the new flag.
| final customPath = | ||
| argResults != null ? argResults![kFirebaseOutFlag] as String? : null; | ||
| // Determine the raw path, prioritizing the programmatic path passed from the configure command. | ||
| final rawPath = _firebaseJsonPath ?? customPath; |
There was a problem hiding this comment.
think we can use similar getter used in config command:
String get firebaseJsonPath {
return resolveFirebaseJsonPath(argResults![kFirebaseOutFlag] as String?);
}| final installDevDependency = Process.runSync( | ||
| 'flutter', | ||
| [ | ||
| 'pub', | ||
| 'add', | ||
| '--dev', | ||
| 'flutterfire_cli', | ||
| '--path=${Directory.current.path}', | ||
| ], | ||
| workingDirectory: projectPath, | ||
| ); | ||
|
|
||
| if (installDevDependency.exitCode != 0) { | ||
| fail(installDevDependency.stderr as String); |
There was a problem hiding this comment.
don't need to install dev, it should be available already. See other tests.
| final result = Process.runSync( | ||
| 'dart', | ||
| [ | ||
| 'run', | ||
| 'flutterfire_cli:flutterfire', | ||
| 'configure', | ||
| '--yes', | ||
| '--project=$firebaseProjectId', | ||
| '--platforms=android', | ||
| '--firebase-out=$customFirebaseJsonPath', | ||
| ], | ||
| workingDirectory: projectPath, | ||
| runInShell: true, | ||
| ); |
There was a problem hiding this comment.
using dev dependency again, see other tests.
| final scriptPath = p.join(Directory.current.path, 'bin', 'flutterfire.dart'); | ||
|
|
||
| // 1. Run "flutterfire configure" with custom output path | ||
| final result = Process.runSync( | ||
| 'dart', | ||
| [ | ||
| scriptPath, | ||
| 'configure', | ||
| '--yes', | ||
| '--platforms=android', | ||
| '--project=$firebaseProjectId', | ||
| '--firebase-out=$customFirebaseJsonPath', |
| final result2 = Process.runSync( | ||
| 'dart', | ||
| [ | ||
| scriptPath, |
3 participants
Fixes #277