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

[native_assets_cli] Drop Config suffixes v2 #1830

Closed
wants to merge 1 commit into from
Closed

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented Dec 18, 2024

Addresses:

A variant of:

This PR normalizes the code and data specific APIs on the config, config builder, output and output builders under a .code and .data extension.

Example link hook code:

void main(List<String> args) async {
  await link(
    args,
    (config, output) async =>
        output.data.addAssets(treeshake(config.data.assets)),
  );
}

Example build hook code:

void main(List<String> arguments) async {
  await build(arguments, (config, output) async {
    if (config.code.targetOS == OS.android) {
      config.code.android.targetNdkApi;
    }

    output.code.addAsset( ... );
  });
}

This drops Config suffixes from the various config accessors.
This changes output.codeAssets.add( to output.code.addAsset(.

The weird quirk in this PR is that it has to provide an API for LinkConfig.code that gives both the CodeConfig getters and an assets getter. I tried using extension types to alleviate this, but extension types can only extend the type they are extending, not the first element of a tuple they are extending.

Separating the linkConfig.code : CodeConfig and linkConfig.assets.code : Iterable<CodeAsset> leads to a less symmetric API though:

Alternatively, we could also opt to land neither of the PRs. And stick with config.codeConfig.androidConfig and output.codeAssets.

@coveralls
Copy link

Coverage Status

coverage: 87.807% (-0.2%) from 87.965%
when pulling 9939a39 on config-names-v2
into 8c16b6c on main.

Copy link
Member

@mosuem mosuem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some nits and more docs - but I like this API.

@@ -188,22 +233,22 @@ extension MacOSConfigSyntactic on MacOSConfig {
/// assets (only available if code assets are supported).
extension CodeAssetBuildOutputBuilder on BuildOutputBuilder {
/// Provides access to emitting code assets.
CodeAssetBuildOutputBuilderAdd get codeAssets =>
CodeAssetBuildOutputBuilderAdd get code =>
CodeAssetBuildOutputBuilderAdd._(this);
}

/// Supports emitting code assets for build hooks.
extension type CodeAssetBuildOutputBuilderAdd._(BuildOutputBuilder _output) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is giving me Java flashbacks - how about CodeAssetCollector?

/// Code asset specific configuration.
CodeConfig get codeConfig => CodeConfig(this);
// Weird class that combines `CodeConfig` getters and a getter for code assets.
class LinkCodeConfig implements CodeConfig {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed a bit ugly, but I don't know a way around it...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem very right to me. It mixes orthogonal concerns (code configuration and outputed assets)

null =>
throw StateError('Cannot access iOSConfig if targetOS is not iOS'
' or in dry runs.'),
IOSConfig get iOS => switch (_iOSConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why a switch instead of a simple if check?

@@ -212,65 +257,73 @@ extension type CodeAssetBuildOutputBuilderAdd._(BuildOutputBuilder _output) {
/// assets (only available if code assets are supported).
extension CodeAssetLinkOutputBuilder on LinkOutputBuilder {
/// Provides access to emitting code assets.
CodeAssetLinkOutputBuilderAdd get codeAssets =>
CodeAssetLinkOutputBuilderAdd get code =>
CodeAssetLinkOutputBuilderAdd._(this);
}

/// Extension on [LinkOutputBuilder] to emit code assets.
extension type CodeAssetLinkOutputBuilderAdd._(LinkOutputBuilder _output) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly verbose as above - and not very understandable...

@mkustermann
Copy link
Member

mkustermann commented Dec 19, 2024

The weird quirk in this PR is that it has to provide an API for LinkConfig.code that gives both the CodeConfig getters and an assets getter.

Based on recent discussions with @mosuem @liamappelbe about what is used to hash the build directory and this PR restructuring, we may want to consider rename a few things and do the following

// build hook
main(...) asyc {
  await build((BuildInput input, BuildOutputBuilder output) {
    input.outputDirectory;
    input.config.{code,...} // <-- only this goes into hashing
    input.metadata.* // <-- metadata emitted from package deps we can utilize

    output.assets.code.add();
  });
}

// link hook
main(...) asyc {
  await link((LinkInput input, LinkOutputBuilder output) {
    input.outputDirectory
    input.config.{code,...}. // <-- only this goes into hashing
    input.assets.{code,data,...} // <-- symmetric to `hook/build.dart`'s `output.assets.{code,data,...}`

    output.assets.code.add();
  });
}

That would achieve

  • very clear separation of configuration (target platform, ...) from input data (metadata, assets to be linked, etc)
  • very clear what goes into build dir hash and what doesn't

This changes output.codeAssets.add( to output.code.addAsset(.

I prefer we don't use addAsset() and just have add() because it's somewhat clear that we're adding assets.
I prefer output.assets.code.add() that groups all assets under one output.assets.

@dcharkes wdyt?

@dcharkes
Copy link
Collaborator Author

Thanks @mkustermann and @mosuem for the input! I've replied to the refactoring suggestion in the original thread.

Overall SGTM. I'll close this PR.

This changes output.codeAssets.add( to output.code.addAsset(.

I prefer we don't use addAsset() and just have add() because it's somewhat clear that we're adding assets. I prefer output.assets.code.add() that groups all assets under one output.assets.

@dcharkes wdyt?

Please see #1829 where I explored that.

@dcharkes dcharkes closed this Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants