-
Notifications
You must be signed in to change notification settings - Fork 60
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_builder] Support pub workspaces 2 #1921
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
|
549efc9
to
79b92d2
Compare
@@ -713,7 +724,7 @@ ${compileResult.stdout} | |||
|
|||
if (input is BuildInput) { | |||
final packagesWithLink = | |||
(await packageLayout.packagesWithAssets(Hook.link)) | |||
(await (await _planner).packagesWithHook(Hook.link)) |
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.
Nested await
s do make my skin crawl a bit ;)
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.
Perhaps
final planner = await _planner;
final packagesWithHook = await planner.packagesWithHook(Hook.build);
further above could be extracted to a method to alleviate this here and in other places, as the _planner
is mostly accessed to run packagesWithHook
. Just a nit.
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.
Agreed with the skin crawl, I already adopted final planner = await _planner;
in other places.
Thanks @mosuem! |
Closes: #1905
This does a bigger refactoring to take
runPackageName
into account when checking for "if there are packages with native assets".It changes the public API in the following way:
PackageLayout
no longer has methods for if packages have hooks, as it does not take into account the dependency graphNativeAssetsBuildRunner
now exposes a method to query whether any packages in the dependencies have build hooksBuildPlanner
, and this class stays internal.)This means that
PackageLayout
has the API to provide the right package layout to theNativeAssetsBuildRunner
, and theNativeAssetsBuildRunner
has the methods to be queried by embedders/launchers.