invertase / flutterfire_cli

A CLI to help with using FlutterFire in your Flutter applications.
Apache License 2.0
164 stars 47 forks source link

fix: Support spaces in paths and product name for apple apps #228

Closed AhmedLSayed9 closed 2 months ago

AhmedLSayed9 commented 7 months ago

Description

If the the app's directory contains space i.e: /Users/Me/With Space/MyApp, current configuration will read the path as /Users/Me and that will cause the configuration to fail when trying to access paths as /Users/Me/firebase.json.

Same issue will happen with product name, so we can't name an application with a space. (https://github.com/invertase/flutterfire_cli/issues/243)

fixes https://github.com/invertase/flutterfire_cli/issues/284

Type of Change

CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

russellwheatley commented 2 months ago

Hey @AhmedLSayed9 - if you want to resolve the file conflict, I would look to get this merged (providing it passes integration tests) ๐Ÿ‘

AhmedLSayed9 commented 2 months ago

@russellwheatley Can you check now?

russellwheatley commented 2 months ago

@AhmedLSayed9 - it isn't going to work, the script now sees those Xcode env variables as Dart variables. Look at the results of the CI run for more information.

AhmedLSayed9 commented 2 months ago

@AhmedLSayed9 - it isn't going to work, the script now sees those Xcode env variables as Dart variables. Look at the results of the CI run for more information.

My bad, I had to escape the '$'. I've fixed them now.

russellwheatley commented 2 months ago

@AhmedLSayed9 - could you write a test like this: https://github.com/invertase/flutterfire_cli/blob/main/packages/flutterfire_cli/test/configure_test.dart#L342-L379

Except change the applePath to one with spaces in it and create a different one for macOS and iOS. Just select platforms as iOS and macOS in the flutterfire configure command and remove checks unrelated to apple platforms.

AhmedLSayed9 commented 2 months ago

@russellwheatley Thanks for the tips. I've added a test case.

AhmedLSayed9 commented 2 months ago

This should also fix #243.

@russellwheatley Can you check if there's still something to add?

russellwheatley commented 2 months ago

@AhmedLSayed9 - thanks for the PR. CI won't run correctly for contributor pull requests. I just tested this locally and it was successful:

Screenshot 2024-04-18 at 11 34 29

Good work ๐Ÿ’ช

rahulraj-idt commented 1 month ago

@russellwheatley This fixes a critical error, shall we have a release for this ? I also faced the same issue.

russellwheatley commented 1 month ago

@rahulraj-idt - This has come out in a dev release: https://github.com/invertase/flutterfire_cli/blob/main/CHANGELOG.md#flutterfire_cli---v101-dev0