flutter icon indicating copy to clipboard operation
flutter copied to clipboard

Bundle `--implicit-pubspec-resolution` working around`flutter_tools` tech debt

Open matanlurey opened this issue 1 year ago • 7 comments

In the arc of work around excluding dev_dependencies-based plugins from release-mode apps, we have run into multiple snags:

  1. Android Flutter apps still support the (legacy) .flutter-plugins format (deprecated since 2019)
  2. We need to be able to deduce what plugins should only be hooked up/built in non-production mode

For (1), we planned to announce a deprecation, give users a flag, and upgrade our own infrastructure:

  • Deprecation: https://docs.flutter.dev/release/breaking-changes/flutter-plugins-configuration.
  • Flag: https://github.com/flutter/flutter/issues/157532
  • Upgrade: https://github.com/flutter/flutter/issues/157660

For (2), we ran into https://github.com/flutter/flutter/issues/102983 and https://github.com/flutter/flutter/issues/73870; where the historic flutter_gen synthetic package is being now overridden by dart pub deps --json, which is being used to figure out which packages come (strictly) from dev_dependencies.

@jonahwilliams's suggestion is to change the flag (--no-emit-legacy-flutter-plugins) to --implicit-pubspec-resolution (name TBD).

The flag would imply:

  1. We no longer generate .flutter-plugins
  2. The flutter generate command is non-operable
  3. The flutter_gen package is output as source instead of as a synthetic package with package_config.json overrides

Background

In Q4, we're updating package:integration_test support to support more native UI interaction and assertions:

  1. Add and augment "native app/view" functionality
  2. Allow taking screenshot on mobile devices and emulators
  3. Add functionality for interacting with/dismissing permission dialogs
  4. Ensure there is sufficient extensibility for external customers

There are two major arcs of work being worked on right now:

  • @johnmccutchan is adding Android UIAutomator support via an integration test plugin: https://github.com/flutter/flutter/issues/157370.

  • @matanlurey is making sure testing-only plugins (i.e. integration_test_plugin) are not bundled in a release of an app: https://github.com/flutter/flutter/issues/56591

matanlurey avatar Oct 29 '24 18:10 matanlurey

Notes:

  • the flutter generate command is currently non-operable, we can ignore it.
  • the flutter gen-l10n command should keep working.

We also need to update https://docs.flutter.dev/ui/accessibility-and-internationalization/internationalization

jonahwilliams avatar Oct 29 '24 18:10 jonahwilliams

I am struggling to understand the consequences of "The flutter_gen package is output as source instead of as a synthetic package with package_config.json overrides"

reidbaker avatar Oct 29 '24 19:10 reidbaker

What incentive would anyone have to affirmatively pass this flag? It doesn't seem like there's much if any direct benefit here, so I wouldn't expect this to be something that more than a vanishingly small number of people would learn about and pass.

If that's the case, we should strongly consider doing this the way we did the Android v1 vs v2 embedding, and make it:

  • First just a warning that this is coming
  • Then change the behavior and temporarily provide a flag people can pass to get the old behavior, with a warning that it will go away.
  • Then remove the flag.

(We should probably not call this "v2" given the Android v2 embedding.)

stuartmorgan-g avatar Oct 29 '24 19:10 stuartmorgan-g

@reidbaker:

I am struggling to understand the consequences of "The flutter_gen package is output as source instead of as a synthetic package with package_config.json overrides"

The consequences are the import path of the generated code changes and it becomes checked-in to version control; in practical terms it's a breaking change, and would require application authors to update their code (and possibly other workflows) accordingly.

For example:

- import 'package:flutter_gen/flutter_gen.dart';
+ import 'package:my_app/src/generated/i10n.dart'; 

void main() {
  // Use symbol(s) from the generated library.
}

To be clear, I'll write another flutter.dev breaking change article with a better explanation as well.


@stuartmorgan:

What incentive would anyone have to affirmatively pass this flag?

Circumstantially larger apps or companies may want to preview how large of a breaking change this is so they can account for it in timelines. For some apps it might be literally a 1-line change (and a git add . && git commit), and others perhaps it is larger change.

I do imagine it would likely be used more aggressively in the negative , that is, as --no-flutter-plugin-v2, to "unbreak" after a breaking change release. However the flagging process is more or less the same, and landing it as an off-by-default flag is not considerably more work, and gives folks more time to make decisions about this change.

If that's the case, we should strongly consider doing this the way we did the Android v1 vs v2 embedding

That is largely our proposal, except that I'd like to add the flag (off by default) instead of only adding a flag later that is on.

An off-by-default flag also lets my team continue to iterate on related changes without waiting for the next stable release.

We should probably not call this "v2" given the Android v2 embedding.

Yeah just to be clear the name proposed above is bad, and is just used as a placeholder. Ideas welcome :)

matanlurey avatar Oct 29 '24 19:10 matanlurey

Based on an offline discussion I'll go with --legacy-pubspec-resolution.

matanlurey avatar Oct 29 '24 19:10 matanlurey

Recognizing that naming is hard and not the most critical part of this work I humbly offer the following.

https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-newold-modifiers-in-code

Maybe --unstructured-pubspec-resolution or --pre-json-pubspec-resolution

reidbaker avatar Oct 29 '24 19:10 reidbaker

I could see --implicit-pubspec-resolution, to signify we were implicity generating a synthetic package and implicitly assuming all plugins were prod plugins. Made the change accordingly.

matanlurey avatar Oct 29 '24 19:10 matanlurey

Unblocked, but let's land https://github.com/flutter/flutter/issues/157929 first.

matanlurey avatar Oct 31 '24 16:10 matanlurey

This is done!

matanlurey avatar Nov 14 '24 22:11 matanlurey

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

github-actions[bot] avatar Nov 28 '24 23:11 github-actions[bot]