Skip to content
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

[Discussion] App modularization #2591

Closed
g123k opened this issue Jul 13, 2022 · 20 comments
Closed

[Discussion] App modularization #2591

g123k opened this issue Jul 13, 2022 · 20 comments

Comments

@g123k
Copy link
Collaborator

g123k commented Jul 13, 2022

Hi everyone,

We now have a few pending issues regarding:

Dart (and Flutter) doesn't allow us to have dynamic dependencies, like Gradle would on Android.
The only solution is to have an architecture that looks like this:

Arch

To achieve this kind of architecture, Flutter forces us to specify the name of the package for each asset we want.
That's why, we have to change the current "app" to be the common module. This will force us to provide a package for all assets we want to access. (That's what I've done in #2320, but not only)

Here is the plan:

  1. Dismiss the current PR feat: New app package for dev purposes #2320
  2. Create a PR that only adds package everywhere in the common module + create an app to test the architecture
  3. Split the barcode scanner in modules: MLKit vs another "non Google" solution
  4. Create two variants of the app

If you have any suggestion, feel free to ask!

@M123-dev
Copy link
Member

I don't know of a better way to do this if we don't have official flutter support.

I mean we could ask if there is a maybe undocumented way or if it's on the todo list but if not I'm fine with doing it this way.

@monsieurtanuki
Copy link
Contributor

Hi @g123k!

If I understand well:

  1. we need to build different apps for Android, because our current version uses Google packages that we cannot/we don't want to put in FDroid or Amazon stores
  2. it's not possible to have a unique smart pubspec.yaml that - according to env variables for instance - would build either the google or the amazon version

I'm a bit puzzled because somehow we manage to override the app version from the pubspec.yaml (0.0.0+600) at build time, so perhaps we could manage to go a step further and override dependencies too. But perhaps not, perhaps we can change only few things, among which app name and version.

So let's assume points 1 and 2 above are correct.

For the moment smooth_app is our main package. And if I understand well you want to rename it as common (or just consider as the "common" package, that's not clear).

What if we said that

  • smooth_app is our most important package, with 95% of the code - we change almost nothing, we just get rid of the camera/scan implementation (like main(AbstractCameraScan scanner))
  • smooth_app_google would be the entry package for the current app, would include package smooth_app, and would have its own implementation of camera/scan for Smoothie and its own additional dependencies
  • smooth_app_amazon would be the entry package for the amazon app, would include package smooth_app, and would have its own implementation of camera/scan for Smoothie and its own additional dependencies

I don't know if that would work. My point here is to have a minimal impact on the existing code.
@g123k I know you're at ease with 99-file PRs like #2320, I'm not a big fan of that (I prefer 99 Luftballons, but that's another story).

So basically, @g123k could you test if we could have

  • new package smooth_app_android that uses slightly modified "smooth_app" but with an ad-hoc implementation of camera/scan (like now)
  • new package smooth_app_amazon that uses slightly modified "smooth_app" but with an ad-hoc implementation of camera/scan (build on ZXing or whatever, I didn't follow that problem)
  • therefore different dependencies for smooth_app_android and smooth_app_amazon, so that we manage to have different apps for different targets using different dependencies but with an untouched code core kept in smooth_app?

@g123k
Copy link
Collaborator Author

g123k commented Jul 18, 2022

So you're right. Let's we rename the current smooth_app to common (but we can keep the same name).
Since it's not the root package anymore, we HAVE TO link all assets manually by specifying the package argument (mainly what's done in #2306

For the tests, no need actually, because I'm 100% sure (I've done it in previous projects).
We will just have to change the entrypoint from smooth_app/main.dart to smooth_app_android|amazon/main.dart

@monsieurtanuki
Copy link
Contributor

Hi @g123k!

There are some things I would like to clarify before you start.

Let's rename the current smooth_app to common (but we can keep the same name)

=> Please do keep the same name. That was the only important point of my previous comment. We need it to keep the code history (at least I do).

Since it's not the root package anymore, we HAVE TO link all assets manually by specifying the package argument (mainly what's done in #2306

Very surprising indeed. That would entail that if I use a pub.dev package that uses its own assets, I would have to rename those package asset files? Obviously some misunderstanding: we need to clarify this. Especially when #2306 has nothing to do with it.

In short, as a test, you would just have to

  • create a new package smooth_fdroid
  • in its main.dart, call smooth_app/main.dart
  • and that's supposed to work, with 0 change on smooth_app, right?

The next step would be

  1. to create an abstract class in smooth_app, something like that:
abstract class AbstractScanner {
  void helloWorld();
}
  1. to extend that class in smooth_fdroid, something like that:
class FdroidScanner extends AbstractScanner {
  @override
  void helloWorld() => print('hi this is Fdroid here');
}
  1. to pass an instance of FdroidScanner as a parameter to smooth_app/main.dart

  2. to start the FDroid app and find hi this is Fdroid here in the logs.

@g123k
Copy link
Collaborator Author

g123k commented Jul 18, 2022

If you download a dependency from pub.dev and this dependency have some assets, you have to specify the package entry. Otherwise Flutter won't know which file to take.

and that's supposed to work, with 0 change on smooth_app, right?

No, because of the package thing

@monsieurtanuki
Copy link
Contributor

@g123k I have to test that. Sounds very strange. I'll keep you posted by tomorrow.

@g123k
Copy link
Collaborator Author

g123k commented Jul 18, 2022

If you want to better understand why, just decompile an Android APK and you will thus understand that package question

@monsieurtanuki
Copy link
Contributor

@g123k We're probably not talking about the same thing.

If I use a package that I find in pub.dev, and if this package contains assets, I do not have to change that package assets, do I?
Like for https://github.com/flutter/packages/tree/main/third_party/packages/cupertino_icons/assets - I don't think you ever changed the path to an icon:
Capture d’écran 2022-07-18 à 19 22 54

Probably the source of our misunderstanding is what we want to do in the old package and the new packages.
The way I see things, the old stays as is with its assets. And the new packages just call the old package with an additional parameter (like a class that implements the scan), but that's all they do. In particular, they don't play with assets, they're calling a package that happens to have its own assets.

Still in my tests.

@g123k
Copy link
Collaborator Author

g123k commented Jul 18, 2022

Let me rephrase everything. Let's say we have:

  • A base app
  • A dependency (cupertino_icons for example)

When in your base app, you say Image.asset(path), it automatically implied package = base_app
=> Current behavior, everything works well (but the package could be provided - actually done by Flutter itself)

Now, Cupertino_icons have its own font asset, but let's say it's an image. If I say Image.asset(path) => 404, because the asset is in a subfolder called cupertino_icons and NOT in the base app.

So if I sum up, in the code (wherever you are), if you want to access an asset that is defined outside the main app, you have to specify the package.

That's what we have to do for smoothie, since the base app will now become a "dependency".

@monsieurtanuki
Copy link
Contributor

if you want to access an asset that is defined outside the main app

I never will.
I will just call a black box - a.k.a. dependency. A black box that uses its own assets.
The main app is never going to access directly the dependency assets.

What you imply is that a dependency does not have the same access to its own assets, depending on whether the dependency is a dependency to another package or is independent. That's possible, but that would be very surprising to me.
I'll continue my tests tomorrow.

@g123k
Copy link
Collaborator Author

g123k commented Jul 18, 2022

No, what I just say, is:

  • Assets from the base app = accessible with or without specifying the package
  • On all cases, specify the name of the package

From the Flutter docs:

To fetch an asset from a package, the package argument must be provided. For instance, suppose the structure above is inside a package called my_icons. Then to fetch the image, use:

AssetImage('icons/heart.png', package: 'my_icons')

@monsieurtanuki
Copy link
Contributor

@g123k My tests today:

  1. in a first approach it looks like you may be right regarding assets. Very surprising - not the fact that you are right, this I can imagine, but that a black box like a dependency needs to be (slightly) rewritten. But if at the end this is how it works, we can manage that in a stupid PR that just adds the path to all asset calls.
  2. what is much more problematic is the way flutter manages the translations (it's notoriously an area where flutter sucks). It looks like as the translations are computed from the current package folder where you flutter run - e.g. smooth_fdroid, all the translations need to be at the level of that folder, as there's only one generated app_localizations.dart file. That would mean that we would need to copy the app_xx.arb files for each fdroid/amazon/google project. Which definitely sucks, much more than point 1

@g123k What are your thoughts about localization?

@teolemon
Copy link
Member

Regarding localization, can we keep everything in a single file ?
The 160*4 files for server and the old Android app are a nightmare.

@monsieurtanuki
Copy link
Contributor

Regarding localization, can we keep everything in a single file ?

You mean, a unique file for all the flutter apps, or a unique file for all languages?

@monsieurtanuki
Copy link
Contributor

@g123k Regarding assets (again), I've just a look at pub.dev packages that contain assets (not that easy to find).
I found this guy: https://pub.dev/packages/intl_phone_field that contains flags. And this is how they access the "local" assets:

Image.asset(
  'assets/flags/${_filteredCountries[index].code.toLowerCase()}.png',
  package: 'intl_phone_field',
  width: 32,
)

Same with https://pub.dev/packages/country_pickers and https://pub.dev/packages/country_list_pick.

OK, you win :)

We'll have to check if all our asset cases (json, svg, png, ogg, pem, txt, ttf) can be dealt with with that additional package parameter.

But as I said, the localizations part looks more problematic.

@M123-dev
Copy link
Member

M123-dev commented Jul 19, 2022

I think the best solution would be to just avoid the problem as we are now in a stage where there are not many people with the same problems.

Preferably by somehow running the generation step before flutter run does it's job like its possible in JS with scripts in the counterpart to darts pubspec.yaml

The closest thing I could find is this:
https://flutterawesome.com/define-and-use-scripts-from-your-pubspec-yaml-file/amp/

For this to work we would still need a command which generates the translations files without running flutter run looked for it multiple times already but haven't found one.


Edit:
I just checked again and it's flutter gen-l10n maybe I should get more in the habit of reading the docs instead of Stack Overflow

@M123-dev
Copy link
Member

Also for other assets there seem to be workarounds https://stackoverflow.com/questions/54740732/how-to-add-assets-in-flutter-package-plugin-development

  1. Moving the ./assets into ./lib/assets

or

  1. AssetImage('assets/abc.xyz', package: 'my_developed_package');

@monsieurtanuki
Copy link
Contributor

@teolemon @g123k @M123-dev You guys realize that ALL that heavy non-sense could be prevented if we were just able to comment and uncomment the pubspec.yaml and a single dart file, right? Either manually or automatically:

  #  smooth_mlkit: ### to be commented/uncommented
  #    path: ../../../smooth_mlkit ### to be commented/uncommented
  smooth_zxing: ### to be commented/uncommented
    path: ../../../smooth_zxing ### to be commented/uncommented

In both smooth_mlkit and smooth_zxing, we create one class called ScanInterface (that possibly implements a common abstract class)

And in smooth_app's main.dart, we just call once ScanInterface:

// import 'package:smooth_mlkit/smooth_mlkit.dart'; ### to be commented/uncommented
import 'package:smooth_zxing/smooth_zxing.dart'; ### to be commented/uncommented

final ScanInterface scanInterface = ScanInterface();

Et voilà.

@M123-dev
Copy link
Member

That should be doable without that much work automatically

@g123k
Copy link
Collaborator Author

g123k commented Apr 8, 2023

I think we can close this issue as it's now implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants