[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