[native_assets_cli] Cleanup constructors

[dcharkes]

Addresses:

• [native_assets_cli] Nest CodeConfig OS-specific config #1824 (comment)

@mkustermann Do you prefer repeating the arguments to CodeConfig in setupCodeConfig leading to the following?

        ..setupCodeConfig(
          targetArchitecture: Architecture.current,
          targetOS: OS.current,
          macOSConfig: targetOS == OS.macOS
              ? MacOSConfig(targetVersion: defaultMacOSVersion)
              : null,
          linkModePreference: LinkModePreference.dynamic,
        );

Or rather avoiding to repeat the arguments and have the following?

        ..setupCodeConfig(CodeConfig(
          targetArchitecture: Architecture.current,
          targetOS: OS.current,
          macOSConfig: targetOS == OS.macOS
              ? MacOSConfig(targetVersion: defaultMacOSVersion)
              : null,
          linkModePreference: LinkModePreference.dynamic,
        ));

I've done the former now, but can easily revert that commit.

[github-actions]

PR Health

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbols

License Headers ✔️

// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

[coveralls]

coverage: 87.524% (-0.003%) from 87.527%
when pulling 8cf42a4 on cleanup-constructors
into 0f9cb0b on main.

[mkustermann]

@mkustermann Do you prefer repeating the arguments to CodeConfig in setupCodeConfig leading to the following?

Firstly & tangentially related, comment from #1824

We never had a CodeConfig constructor that took it's constituents, because protocol extensions have a setup method.

This was by-design. The thinking behind it was that API is mainly focused on hook authors, so it's focused on reading the json and were therefore basically views on the json.

The only exceptions were "leaf" nodes such as Architecture/OS/CompilerConfig. The difference between the "leaf" nodes and the others in the API is that the leaf nodes are closed, they are not extensible (e.g. a CCompilerConfig has fixed number of fields and it's toJson is only serializing those, not any others - so it cannot be extended).

So for all API classes that are effectively read-only views on the json, they are extensible: They know only about some fields, but the json may have extensions they don't know about. It's not a problem because a) they don't take individual fields in constructor b) they don't support toJson() (both of which requires knowing all the fields). The prime example is e.g. BuildConfig (if it took all constituents as arguments, it would need to know all constituents and would therefore not be extensible)

So by making CodeConfig not a view on a json but also let it take all constituents as arguments in the constructor, it makes the class sort of closed, not extensible.

Now we had more discussions somewhere recently about extensibility. In general we may not want to have extensions on extensions, but it could be that in some cases the added flexibility may come in handy sometimes).

So from that angle I'd lean on making the API classes views on the json unless we really have strong arguments for making them closed (closed == adding constructor with all constituent fields, adding toJson() - which make them no longer be backed by json).

Looking at this PR, I see that the only reason we currently need CodeConfig(HookConfig hookConfig) (or with this PR CodeConfig with constituent parts) instead of CodeConfig(Map<String, Object?> json) is that the decoding logic needs hookConfig.dryRun.

=> If we first prune all code of dry run (all users can upgrade to newest dart / flutter main/dev branches) then it seems to me CodeConfig can be simply a view on the json and constructed via CodeConfig.fromJson

@dcharkes would that work?

Otherwise the change LGTM

[dcharkes]

Both before this PR, CodeConfig was a class with its constituent members, you introduced this in the big refactoring in 5164777. Also in #1824 (comment). If I understand you correctly, you're saying our progressive insight on how this should work should move more towards things only having a json field.

Which means we have getters that simply return another view on a JSON.

Looking at this PR, I see that the only reason we currently need CodeConfig(HookConfig hookConfig) (or with this PR CodeConfig with constituent parts) instead of CodeConfig(Map<String, Object?> json) is that the decoding logic needs hookConfig.dryRun.

It has access to dryRun anyway, because it has access to the "root" of the Json, rather than the nested json (*). I've made the constructor private now (the alternative is making all fields late and merging the factory and private constructor).

In a subsequent refactoring I remove the buildAssetTypes from setupHookConfig and let the setupCodeConfig add 'code' to the root buildAssetTypes, that will require still having access to the root json in the builder. We could potentially avoid that if we completely remove buildAssetTypes and mandate that the config contains an empty data: {} DataConfig if data assets need to be built.

This was by-design. The thinking behind it was that API is mainly focused on hook authors, so it's focused on reading the json and were therefore basically views on the json.

Authors also want to test their hooks, so they are also going to use the ..setupCodeConfig. But indeed we don't need a constructor with its constituents if they have the setup method that takes constituents.

(*) There is currently no nested json yet.

---

Some whiteboarding:

We could consider instead of having:

builder
  ..setupHookConfig(...)
  ..setupBuildConfig(...)
  ..setupCodeConfig(...)
  ..setupDataConfig(...);

having this:

builder.setup([
  HookConfig(...),
  BuildConfig(...),
  CodeConfig(...),
  DataConfig(...),
]);

Where all these classes implement an interface have an applyTo(json) method. That would kind of line up the constructors with the getters. The constructors, even if they take all the constituents, could just build an internal json that's applied to the config json on applyTo. But not sure how much that gains us over just having the setup methods. The constructor might be easier to find than an extension method. But the extension methods seem cleaner on the call site, less braces, and no reasoning about an apply method.

[mkustermann]

Both before this PR, CodeConfig was a class with its constituent members, you introduced this in the big refactoring in 5164777. Also in #1824 (comment). If I understand you correctly, you're saying our progressive insight on how this should work should move more towards things only having a json field.

Which means we have getters that simply return another view on a JSON.

Yes, it pulls out the parts (it knows about) out of the json immediately (to get syntax errors) and put them in fields.
As soon as we have a use case for extensibility it would just have an additional Map<String, Object> json field. There was no need for this atm, but it's easy to add once we have a need for it.

=> This is exactly how HookConfig etc work today: Eagerly validate the fields it knows and store in fields. Have a json field iff we have a need for it.

So I don't think this is progressive insight, it's just how it was intentionally designed in my refactoring.

[mkustermann]

In a subsequent refactoring I remove the buildAssetTypes from setupHookConfig and let the setupCodeConfig add 'code' to the root buildAssetTypes, that will require still having access to the root json in the builder.

That's somewhat a non-local side-effect. While one can see reasons why one may want it, the pattern of a setupX method not only modifying X but other things is IMHO a bit of a smell. Also it then requires setupX to get access to global root of json, which is also a bit of a smell.

I think for embedders to supply buildAssetTypes explicitly is not very complicated, so why change it?

Where all these classes implement an interface have an applyTo(json) method.

Yes, an applyTo(json) mechanism would continue to allow extensibility - compared to toJson() which has to know all fields.

Though if this method is on API classes (e.g. on HookConfig): Those are already views on a json. So an applyTo(json) would simply pull it out of that json and put it into another json? But a user can already use HookConfig.json directly, why an applyTo(json).

It would then probably builder classes (e.g. HookConfigBuilder) that have applyToJson. But that's exactly what they already do: They have a json and the setupX methods initialize it.

[mkustermann]

Consider keeping the old code here and renaming this to

// TODO: Merge with `.fromJson` once dry-run is gone.
CodeConfig._fromJsonAndDryRun(Map<String, Object?> json, bool dryRun) {}

factory CodeConfig.fromJson(Map<String, Object?) json) {
  return CodeConfig._fromJsonAndDryRun(json, json.getOptional<bool>(_dryRunConfigKey) ?? false);
}

Otherwise there's high chance someone may want to make it public in the future.

[dcharkes]

Map<String, Object?> json, bool dryRun

There is no nesting currently, so dryRun is in the same root level as the code config fields.

Otherwise there's high chance someone may want to make it public in the future.

Left a comment to avoid someone doing this.

[dcharkes]

So I don't think this is progressive insight, it's just how it was intentionally designed in my refactoring.

I'm confused, why didn't you do this with CodeConfig in your refactoring then?

That's somewhat a non-local side-effect. While one can see reasons why one may want it, the pattern of a setupX method not only modifying X but other things is IMHO a bit of a smell. Also it then requires setupX to get access to global root of json, which is also a bit of a smell.

Yeah, I think that's a good reason to get rid of buildAssetTypes and rely on the code and data fields as indicators for those asset types once we properly nest.

It would then probably builder classes (e.g. HookConfigBuilder) that have applyToJson. But that's exactly what they already do: They have a json and the setupX methods initialize it.

Yeah, indeed.