[native_assets_cli] NativeCodeAsset and DataAsset should accept an assetId instead of package and name.

[mkustermann]

Currently there is asymmetry between hooks and runtime

• the hooks use e.g. NativeCodeAsset(package: ..., name: ...)
• the runtime use @Native<Void Function()>(assetId: '...')

we should remove this asymmetry by only requiring an assetId everywhere.

[dcharkes]

This was changed in #946 based on feedback from @mosuem that having the package:$packageName/$name is bad usability.

If I remember correctly, the wish was to have the package name be automatic. But since the assets are data classes, and we have a single add method for assets rather than a new add method for BuildOutput for every asset, reading the package name from somewhere doesn't work.

I also don't mind having assetId everywhere instead. It also simplifies link hooks if they don't want to modify the assetId.

We could have an extension type AssetId on String that gives you the package name and asset name. And have a constructor that makes it take a package name and asset name. That would avoid users having to do string concatenation.

[mkustermann]

Users of this system will write hook/build.dart and lib/foo.dart and it's very confusing if the two don't use the same way to identify assets.

And using const DataAsset(package: 'foo', name: 'bar') is IMHO worse than const DataAsset('package:foo/bar'). Also the ladder allows for non-package uris in the future, which the former doesn't.

[mosuem]

using const DataAsset(package: 'foo', name: 'bar') is IMHO worse than const DataAsset('package:foo/bar').

I disagree. Using a "non-typed" API by requiring a string in a certain format is (IMO much) worse than making the format explicit.

Also the ladder allows for non-package uris in the future, which the former doesn't.

Agree there.

If I remember correctly, the wish was to have the package name be automatic.

Yes, I think it is weird to ask the user to supply information we already have. The user should just choose the name, then the point about non-package uris would also be moot.

[mkustermann]

using const DataAsset(package: 'foo', name: 'bar') is IMHO worse than const DataAsset('package:foo/bar').

I disagree.

const DataAsset(package: 'foo', name: 'bar') is to me as a user clearly

• harder to type on a keyboard
• harder to read

(it has colons, quotes, commas spaces) than const DataAsset('package:foo/bar').

It doesn't prevent errors either, because I may supply the wrong package or names values. The only thing it really does is it adds a package: in front.

If we have an invariant that any asset ID must start with package: then one could even think of leaving that part away, since it's redundant information.

I understand the urge to model things very detailed with Dart classes, but it often leads to over engineered and hard to use APIs. (Tangential example: We used to have a Path class representing file system paths, it had e.g. Path.components for things between directory separators, ... - it tried to model in Dart classes what a file path looks like - it has been super cumbersome to use and nowadays all code uses package:path and strings. I think it's somewhat similar here.)

Using a "non-typed" API by requiring a string in a certain format is (IMO much) worse than making the format explicit.

The "format" is a ID for which a String seems appropriate.

The fact that currently the system restricts it to be package: uris is a self-imposed restriction that isn't necessary. Actually I do find it confusing to use that in the first place, because

• users may think they can const DataAsset('package:intl/intl.dart').load() (normally package uris locate things inside the lib/ folder - but not for the asset package uris, which is confusing)
• higher-level abstractions may want to use different naming scheme e.g. const ShaderAsset('shader:flutter/circle')
• The existing @Native() API also takes a opaque String assetId
• Flutter's asset APIs also take simple strings

If I remember correctly, the wish was to have the package name be automatic.

This asymmetric API is super confusing and I'm strongly opposed to this (me as a user of this have made mistakes due to this asymmetry several times). We should have hook/build.dart use the same names as in lib/foo.dart. And the package name cannot be automatic on the usage site, so it has to be explicitly written.

(Strongly in favor of KISS design principle - keep things simple, not over-engineer APIs, ...)

[mosuem]

We should have hook/build.dart use the same names as in lib/foo.dart
Strongly in favor of KISS design principle - keep things simple, not over-engineer APIs, ...

Agree.

me as a user of this have made mistakes due to this asymmetry several times

Also agree - my problem is that I am always unsure what to type in String fields - does this want the name? The name prefixed with the package name? That package name prefixed with package:? I believe using named parameters for the individual components gets rid of this.
I am completely fine with a single assetId field, as long as there are no restrictions on what I can type in that field. If the field is a string, any string should be ok as an input. That is what the type String says. Obviously, the compilation throwing in case of colliding assetIds is still fine.

I understand the urge to model things very detailed with Dart classes, but it often leads to over engineered and hard to use APIs.

I agree that one has to be careful with this. In this case, we are talking about two components, so it's fine to model things in a more detailed way.

If we have an invariant that any asset ID must start with package: then one could even think of leaving that part away, since it's redundant information.

Maybe that is the issue to tackle here first before deciding on the API. If we do want this restriction, we should auto-fill that information when adding assets, and have a named argument package for loading assets, so that users only ever use the name to identify assets.
If we don't, then the identifier should be a "free-form" string field.

[dcharkes]

If we have an invariant that any asset ID must start with package: then one could even think of leaving that part away, since it's redundant information.

Hm, that's interesting. But that would break the symmetry omitting @DefaultAsset() and @Native(assetId and not omitting it. Because when they are omitted we default to the Dart library URI. (Or we need to change that to the Dart library URI but without the package:?)

Build hooks may only report assets that start with package:<your-own-package-name>/. This is to force (instead of nudge) to avoid naming conflicts between asset id's in various packages. We do allow linkers to pass through asset id's untouched of other packages. Also we allow native code assets and data assets to be used by dart code in a different library.

Since the reporting is restrictive in the build hook but not in the link hook and not in uses, and if we do want symmetry in reporting and using, we redundantly have to repeat <my-package-name>/ in the build hooks.

'shader:flutter/circle'

That's not allowed currently. Maybe we should? Or maybe we should clamp down our design and say we're never going to allow other schemes than package:?

[mkustermann]

'shader:flutter/circle'

That's not allowed currently. Maybe we should? Or maybe we should clamp down our design and say we're never going to allow other schemes than package:?

One can make this just fine. Because this is passed to a high-level API which is implemented in terms of a lower-level one, this implementation has to eventually bottom out on a data asset and use it's naming convention.

You can see an example in my recently sent out CLs pkg/asset/lib/hook.dart & pkg/asset/lib/asset.dart which re-writes asset names from higher level names to lower level ones.

I think one would want to have it like this because a const IntlData('<id>') and const Shader('<id>') should be two different things, defining a intl asset with <id> should not allow loading it as a shader, so higher level abstraction will

• provide hook helpers that internally mangle the id
• provide runtime implementation that internally mangles the id

Thereby ensuring that if you have a higher-level asset (intl, shader, ...) you can only load it using those higher-level apis (intl api, shader api) and you'll get the tree shaking as you're supposed to.

=> Want to effectively have a different ID namespace for every higher level, tree shakeable abstraction.

[dcharkes]

Notes from discussion today. The majority prefers having separate arguments/getters over exposing the assetID string. And we should ensure that we then do the same everywhere. A hopefully exhaustive list:

• We need a $package name somewhere in the asset ID to prevent collisions between assets from different packages. The embedder/storage format should to enforce this.
• We need a user-defined $type in the asset ID to prevent collisions between different types of assets once users start building abstractions on top of DataAsset. The user-defined abstraction should enforce this. The Dart team can provide an example.
• For the storage format, these need to be combined into a string and be deserializable again to provide the 3 components: package, asset name, and (optional) user-defined asset type. One way to do this would be package:$packageName/$assetName and package:$package/$type:$name.
  • There is no 100% guarantee that users reverse engineer the name mangling. And we can't enforce it in the protocol/embedder. Packages should be able to create any asset type defined by any of their dependencies, so there is no encapsulation similar to the package name that can be checked.
  • The "package:" is boilerplate for data assets. This boilerplate makes defaulting assetIds to the library URI work with native code assets. We could opt to diverge data assets and code assets, but it's only in the storage format anyway.
• For the API in the base layer, we can have 2 String arguments and getters in the base API (asset name and package).
• For the API in the user-defined layers we can have the same two arguments. This user-defined abstraction layer will put $type:$name inside the lower layer name.
• In order for the error messages for native code assets to make sense and for the default assetIds from dart:ffi to work, native code assets should have also an assetId getter.
• The data assets in the package:asset should be looked up with (String package, String name). And then we can hide the assetId getter from DataAsset in this package. cc @mosuem

I've cooked up some CLs to change the API in this package:

• [native_assets_cli] asset ids option 1 #1536
• [native_assets_cli] asset ids option 2 #1538
• [native_assets_cli] asset ids option 3 #1539

[mkustermann]

I've cooked up some CLs to change the API in this package:

• [native_assets_cli] asset ids option 1 #1536
• [native_assets_cli] asset ids option 2 #1538
• [native_assets_cli] asset ids option 3 #1539

We should set us up for a future, where on the lowest level, we support various kind of asset types. Those asset types may be extensions to the basic build/hook protocol. None of the asset kinds are any more special than others, they should all be on same footing. Because of this, I think we should decouple code asset from data asset - they don't have to share a common class hierarchy with specific members for example.

(Imagine a package:flutter_build_hooks that provides a flutter-specific build config (e.g. gradle version) and flutter-specific asset kinds (e.g. proguard rules). We wouldn't want to make that inherit from a shared Asset class and require it to have an Asset.id. We may also add support for other asset kinds that won't have an Asset.file.)

=> So the first step I think is to decouple the data assets from code assets.
=> It should be possible for 3rd parties to extend the protocol to new asset types (that extensibility should work on the JSON protocol itself as well as on the APIs we design here that abstract over the JSON)
=> #1539 requires all assets to have Asset.{id,file} which we may not want to require
=> #1538 requires all assets to have Asset.{id,package,name,...} which we may not want to require
=> #1536 requires all assets to have a Asset.file which we may not want to require (it's nullable yes, but the concept of file may not make sense for some 3rd party asset types)

We may not want to model this as a Asset class hierarchy at all.

(It could possibly be buildOutput.dataFiles.add() / buildOutput.nativeCode.add() - and 3rd parties can add buildOutput.proguardRules.add() )

Then the next step is to decide for all asset kinds individually what the best hook API and runtime API looks like - it may choose a different way for code assets and data assets. But I think for a specific asset kind the hook API and the runtime API should refer to them the same way (i.e. symmetric API) to make it easy for users to understand the connection between their hook/build code that adds something and their lib/* that uses it.

There are considerations that may lead to different outcomes for the code & data assets:

• Code Assets: They may (to be figured out!!!) form a global name space (@dcharkes do I recall this correctly that native libraries currently end up in the same folder without renaming and are therefore globally namespaced?): If package A emits a libfoo.so, a package B cannot emit libfoo.so - it would be a collision. Also the exported symbols in libfoo.a and libbar.a cannot overlap. That would be how it's done in C and it would allow taking static archive files from the build of all transitive packages and statically linking them together - as it would be in C. It would allow a bundling tool like flutter, to put all shared libraries in the same folder (instead of a folder per package or renaming shared libraries to include package name and re-writing install names, dependency names, ...)
  => If we go this route then code assets may be a global name space, not sub-namespaced per package. We wouldn't require the package name - neither in the @Native() annotation nor in the hook/build.dart code asset API - it would be symmetric.
  => In fact right now it seems we don't enforce @Native() annotations to have package:* uris as their ids (at least not in the Dart code, maybe we do enforce on the hook/build.dart side?)
  => If it's possible to namespace native code per package properly, we may do that - but the point here is that we can decide that individually for code assets, independent of the decisions we make for data assets (or proguard assets, ...)

• Data Assets: We want to have them namespaced per package. So the high-level APIs (e.g. const ShaderAsset() / const StringDataAsset()) either have to require users to specify it explicitly, or we (in restricted cases) deduce it automatically - 3rd parties can decide on their own, but should follow the general principle
  => If we deduce it automatically we should do so in the high-level runtime API and the high-level hook API
  => If we require explicit package name we should do so in the high-level runtime API and the high-level hook API
  => It should be symmetric
  For the low-level data asset API (both the hook/build API and the runtime loading API - should be symmetric) we may could require them as separate arguments (e.g. buildOutput.files.add('pkgname', 'idname', ...) and loadStringAsset('pkgname', 'idname')). It may not matter much how exactly it's done because this is the low-level API that very few developers will use, most will use a higher-level API that may make their own choice (e.g. output.shaders.add('baz') / const Shader('baz') with automatically deduced package name).

(Apologies for the long comment)

[dcharkes]

form a global name space

Yes. We can rename files and rewrite dynamic linker imports for iOS/MacOS, but not for Linux. #1425

We wouldn't require the package name - neither in the @Native() annotation

How do you envision the default asset ids to work in this scenario if we don't default to library URI.

We may not want to model this as a Asset class hierarchy at all.

Before we end up doing that rewrite, should we land any of the 3 PRs, if not I'll just close them and leave it beautifully asymmetric until we get to that rewrite. As discussed offline, the rewrite of the Dart API is less urgent than getting the JSON protocol correct.