feat: make the cli compatible with jaspr projects - #440
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the isFlutterCompatiblePackage property to the Package class, enabling packages that depend on 'jaspr' to be recognized as Flutter-compatible. The isFlutterApp logic was updated to include these compatible packages, and new unit tests were added to verify the classification of various package types. I have no feedback to provide.
|
I don’t think this PR is the right solution, even though it fixes the immediate Jaspr case. The core issue is that it changes the definition of “Flutter app” globally by treating any package with a jaspr dependency as app-compatible in package.dart. That is too specific to one framework, and it means every future non-Flutter host would need another hardcoded special case here. More importantly, isFlutterApp is the predicate that unlocks the rest of the CLI surface, not just configure (package.dart, base.dart). That has broader effects than the PR description suggests: configure is explicitly described as “Configure Firebase for your Flutter app” in config.dart. It does more than generate Dart options: it also validates native inputs, fetches platform configs, writes Android/iOS/macOS files, and writes firebase.json state for later replay (config.dart, config.dart, config.dart, config.dart). I think a better direction would be to keep flutterfire configure as the host-app integration command and introduce a separate command, something like That alternative could focus on dart specific integration (e.g. There is already common code in the current configure flow that could be reused for this: Dart/web/windows/linux Firebase app lookup and config conversion in firebase_dart_options.dart That would let us support runtime config generation for package-style or alternate-host projects without redefining them as Flutter apps and without unintentionally enabling reconfigure, update, and other app-specific behavior. |
|
hI @russellwheatley - Can you elaborate a bit on how the experience would then be for a Jaspr developer using the CLI with this recommended approach? |
|
Hey @abdallahshaban557 - the diff would be: flutterfire configure-runtime + argumentsrather than: flutterfire configure + arguments
flutterfire configure-runtime path=/path/to/dart_options.dart appId=firebase-web-app-id --overwrite-existingCould even create an app on their Firebase project and/or create Firebase project like configure command if they wish. |
|
Hi @russellwheatley, I like this direction and it makes sense, I also like that it opens the door for future surfaces we might want to support - which in the long term I definitely want to pursue. @schultek - got any thoughts on this direction? Can you update this PR accordingly? |
|
Thanks @russellwheatley for looking into this. The proposed changes I made are with the goal to have the least possible change in the package, but I'm open to other approaches. Maybe instead of adding a separate command, we can abstract an Then we have more specific control over what things are done for each command without needing separate |
|
@schultek - that could work. I think I'd prefer to call it How do you propose to invoke that cli path? |
I would still detect Jaspr as a dependency, like it's done with flutter.
I'm not feeling strongly about this, but I'd probably prefer just a warning that a flag will be ignored / isn't supported in that cli path. |
|
@russellwheatley @abdallahshaban557 I've updated the approach, lmk what you think. |
|
Hi @russellwheatley - can we please check on this PR next week? |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
@russellwheatley @abdallahshaban557 Could we get some eyes on this please? |
|
Checking with Russell, Kilian. Will get back to you! |
russellwheatley
left a comment
There was a problem hiding this comment.
Thanks @schultek. This seems better than the original approach - moving from a single global isFlutterApp flag to an App capability model addresses my earlier concern that widening the flag would unintentionally unlock reconfigure/update/install behavior. Having configure key off flutterApp.android/.ios/.web per-command rather than one blanket boolean seems better to me than a single flag.
A few things inline before this can land, plus one design question on extensibility.
| jaspr: ^0.23.0 | ||
| '''), | ||
| ); | ||
| expect(package1.isFlutterCompatiblePackage, isTrue); |
There was a problem hiding this comment.
Tests don't compile right now, this is why CI is red on all 3 platforms:
test/unit_test.dart:285:23: Error: The getter 'isFlutterCompatiblePackage' isn't defined for the type 'Package'.
I think this should be isRuntimePackage. Same issue on line 295.
| jaspr: ^0.1.0 | ||
| '''), | ||
| ); | ||
| expect(jasprApp.isFlutterApp, isTrue); |
There was a problem hiding this comment.
This assertion looks wrong regardless of the naming issue above. jasprApp only depends on jaspr (no flutter/flutter_localizations), so per the current isFlutterApp impl this should be isFalse, with isRuntimeApp being the one that's true.
| void commandRequiresFlutterApp({bool allowRuntimeApp = true}) { | ||
| if (flutterApp == null) { | ||
| throw FlutterAppRequiredException(); | ||
| } | ||
| if (!allowRuntimeApp && flutterApp is FlutterRuntimeApp) { | ||
| throw FlutterAppRequiredException(); | ||
| } | ||
| _warnUserIfRunningGlobally(); |
There was a problem hiding this comment.
Nothing calls commandRequiresFlutterApp(allowRuntimeApp: false) anywhere in config.dart/reconfigure.dart/update.dart/install.dart. If there's no command that should reject FlutterRuntimeApp, maybe drop this parameter until there is one.
|
|
||
| /// Returns whether this package is a runtime package. | ||
| /// | ||
| /// These are packages that are compatible with Flutter plugins, even | ||
| /// if they don't depend on the Flutter SDK directly. | ||
| /// | ||
| /// This is currently only true for packages that depend on Jaspr. | ||
| late final bool isRuntimePackage = dependencies.contains('jaspr'); |
There was a problem hiding this comment.
I think we should support Jaspr, but this detection is hardcoded to match just that. Supporting a second non-Flutter host later means adding another hardcoded dependency check here plus another subclass in flutter_app.dart.
| final package = await Package.load(appDirectory); | ||
| if (!package.isFlutterApp) { | ||
| return null; | ||
| if (package.isFlutterApp) { | ||
| return FlutterProjectApp(package: package); | ||
| } | ||
| return FlutterApp( | ||
| package: package, | ||
| if (package.isRuntimeApp) { | ||
| return FlutterRuntimeApp(package: package); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Since we're introducing "runtime apps" as a first-class concept rather than a Jaspr-only special case, could this be a small registry of adapters instead, something like a list of {detect: (Package) => bool, capabilities: ...} entries that load iterates over, so the next framework is additive rather than another edit here? Doesn't need to be elaborate, just enough that Jaspr isn't the only thing this can ever recognize.
|
As an FYI, I've merged with main to see latest CI run |
Description
Resolves #421
This makes the Cli not throw the
FlutterAppRequiredExceptionexception when run in a Jaspr project, since Jaspr has support for running Flutter plugins such as the Firebase plugins.CC @abdallahshaban557 as discussed
Type of Change
feat-- New feature (non-breaking change which adds functionality)fix-- Bug fix (non-breaking change which fixes an issue)!-- Breaking change (fix or feature that would cause existing functionality to change)refactor-- Code refactorci-- Build configuration changedocs-- Documentationchore-- Chore