-
Notifications
You must be signed in to change notification settings - Fork 53
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] Fix system libraries and add documentation #1880
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
|
@@ -198,7 +198,7 @@ void _validateCodeAssets( | |||
} | |||
|
|||
final file = codeAsset.file; | |||
if (file == null && !dryRun) { | |||
if (file == null && !dryRun && _mustHaveFile(codeAsset.linkMode)) { |
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 fix is maybe 10 lines of code. All the other things in here are 350 lines of code. I think this is a huge burden to maintain. Why can't we have one simple package, that emits various kids of code assets and executes them at runtime? Then the test in this PR would be maybe 30 lines of code (10 for a hook, 10 for native declarations and 10 for invoking and checking) instead of 10x this. The tests would also run much faster. Any good argument that speaks against this?
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.
350 lines:
- Example to illustrate how this works, to cover a user journey for Mariam.
- Integration test combines all the link modes that are not the bundling mode.
and executes them at runtime
Yeah, that's not going to work until the fix rolls in to Dart and Flutter, so that's why test_data/
is exercised in dartdev
. I'll make sure we exercise it in Dartdev on the roll.
I think this is a huge burden to maintain.
Objection noted. Agree to disagree with refactoring tools at our disposal.
Closes: #940
Closes: #1879