-
Notifications
You must be signed in to change notification settings - Fork 74
[hooks] Run link hooks in order #2417
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
base: main
Are you sure you want to change the base?
Conversation
PR HealthBreaking changes ✔️
Changelog Entry ✔️
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.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
89d0f4f
to
031f3ee
Compare
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General approach LGTM. I left a bunch of detailed comments.
2f596d8
to
fb5a45b
Compare
@@ -185,11 +185,19 @@ final class LinkOutputCodeAssetBuilder { | |||
|
|||
LinkOutputCodeAssetBuilder._(this._output); | |||
|
|||
/// Adds the given [asset] to the link hook output. | |||
void add(CodeAsset asset) => _output.addEncodedAsset(asset.encode()); | |||
/// Adds the given [asset] to the hook output with [routing]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Routing deserve some better documentation. For a developer seeing this for the first time it is unclear what routing is and there are no breadcrumbs in the docs to find out more about it.
Questions the docs should answer:
- What is routing?
- Why/When should I set it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think documentation is lacking in general - so far there are little to no starting points for developers. I will also add some here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through this, it seems kind of well documented though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected to see some tests that call the add
/addAll
API while providing a value to the new routing parameter to test that its plumped through correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be covered by the tests of pkgs/hooks_runner/test_data/flag_app
pkgs/data_assets/pubspec.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be new tests in this package to cover the new functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be covered by the added test at pkgs/hooks_runner/test/build_runner/link_test.dart
pkgs/hooks_runner/test_data/fun_with_flags/lib/src/fun_with_flags_base.dart
Outdated
Show resolved
Hide resolved
pkgs/hooks_runner/test_data/flag_enthusiast_2/lib/src/flag_enthusiast_2_base.dart
Outdated
Show resolved
Hide resolved
pkgs/hooks/lib/src/config.dart
Outdated
}) { | ||
_syntax.setup( | ||
assets: [ | ||
for (final asset in assets) AssetSyntax.fromJson(asset.toJson()), | ||
], | ||
assetsFromLinking: [ | ||
for (final asset in assetsFromLinking) | ||
AssetSyntax.fromJson(asset.toJson()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we are doing this above as well, but it seems strange that we are converting something to json only to decode it from json again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume this is from the input/output json files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't assume, verify. (Git blame is your friend.)
As for the JSON ecoding
EncodedAsset
is an extension point, so theencoding
field is not decoded
bypackage:hooks
but bypackage:code_assets
for example.- Before [native_assets_cli] Cleanup JSON: old asset encoding #2220, the encoding could be in
two places, but not anymore. So this can be cleaned up.
[hooks] Cleanup encoded assets encoding #2454 - We still need to copy the map on
toJson
/fromJson
due toEncodedAsset
overriding hashCode and equals. ([native_assets_cli] PreventEncodedAsset
andMetadata
hashcode changing #2131)
088820a
to
ed6e124
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to cover the unhappy paths with tests as well.
- cyclic dependencies with link hooks (and only one build hook, so no cycles for the build hooks)
- outputting an asset for a link hook not directly in the dependencies, that should not be visible. (Actually we should probably make that an error, but it looks like
pkgs/hooks/lib/src/validation.dart
does not throw an error on that right now.)
pkgs/hooks/lib/src/config.dart
Outdated
}) { | ||
_syntax.setup( | ||
assets: [ | ||
for (final asset in assets) AssetSyntax.fromJson(asset.toJson()), | ||
], | ||
assetsFromLinking: [ | ||
for (final asset in assetsFromLinking) | ||
AssetSyntax.fromJson(asset.toJson()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't assume, verify. (Git blame is your friend.)
As for the JSON ecoding
EncodedAsset
is an extension point, so theencoding
field is not decoded
bypackage:hooks
but bypackage:code_assets
for example.- Before [native_assets_cli] Cleanup JSON: old asset encoding #2220, the encoding could be in
two places, but not anymore. So this can be cleaned up.
[hooks] Cleanup encoded assets encoding #2454 - We still need to copy the map on
toJson
/fromJson
due toEncodedAsset
overriding hashCode and equals. ([native_assets_cli] PreventEncodedAsset
andMetadata
hashcode changing #2131)
@@ -250,6 +250,12 @@ | |||
"$ref": "#/definitions/Asset" | |||
} | |||
}, | |||
"assets_from_linking": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cover this in pkgs/hooks/test/json_schema/schema_test.dart
Also, the guard for assets_for_linking
should not skip link output anymore.
You'll need to also add test data for the schema in pkgs/hooks/test/data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand that test - is there documentation for what it is testing somewhere? Or do you know what has to be changed to make it pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test loads data, checks no validation errors, then modifies the data, and checks that the validation finds an error.
All the tests print the schema and the data file used in the test name:
file:///Users/dacoharkes/src/dart-lang/native/pkgs/hooks/doc/schema/hook/link_input.generated.schema.json file:///Users/dacoharkes/src/dart-lang/native/pkgs/hooks/test/data/link_input.json assets.assets_from_linking.1.encoding.key missing
You can run the tests in the debugger to figure out why they are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Or, eyeball it with git blame on the history of the test file 😆 )
dce41a1
to
15cf956
Compare
Changing |
Because it's user-facing in the hooks as well? Maybe skip it for now and keep the version |
Fixes #2381
This brings an order in which the link hooks are run - reverse to the build hook run order. This starts at the application link hook, then it's dependencies, and so on.
This enables us to pass information from on link hook to another as
MetadataAsset
s - but also means that now link hooks must be invoked, regardless of whether assets are sent to the from a build hook.Seems to work: https://dart-review.googlesource.com/c/sdk/+/441201
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.