Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[native_assets_cli] Use a schema and schema checker for hook config and output #1826

Closed
dcharkes opened this issue Dec 18, 2024 · 0 comments · Fixed by #2126
Closed

[native_assets_cli] Use a schema and schema checker for hook config and output #1826

dcharkes opened this issue Dec 18, 2024 · 0 comments · Fixed by #2126
Labels
package:native_assets_cli type-code-health Internal changes to our tools and workflows to make them cleaner, simpler, or more maintainable
Milestone
Native Assets v1.x

Comments

@dcharkes
Copy link
Collaborator

We should consider

  1. generating a an implementation for the build/link config and build/link output rather than handwriting it
  2. use some kind of schema spec to generate from and check backwards and forwards compatibility (due to version skew between the Dart SDK and hook helper packages).

The schema evolution checks will validate we don't introduce any syntactic errors.
The validators for different parts of the config / output validate semantic errors.

Since we don't validate syntactic errors, we should not do any breaking changes to syntax (only when we can bump an SDK constraint). For example, nullable fields in the schema should always stay nullable, even if semantically they are guaranteed to not be null under certain conditions. See the discussion on #1824, thanks @mkustermann!

We can introduce a schema definition, a schema evolution checker, and a code generator from that schema. Then we can make the semantic API (the user API) take the syntactic classes as input (rather than the JSON), and ditto for the validator functions.

Related:
@dcharkes dcharkes added type-code-health Internal changes to our tools and workflows to make them cleaner, simpler, or more maintainable package:native_assets_cli labels Dec 18, 2024
@dcharkes dcharkes added this to the Native Assets v1.x milestone Dec 18, 2024
@dcharkes dcharkes mentioned this issue Jan 31, 2025
Closed
@dcharkes dcharkes mentioned this issue Mar 5, 2025
Merged
This was referenced Mar 17, 2025
Merged
Merged
auto-submit bot pushed a commit that referenced this issue Mar 17, 2025
@dcharkes
[native_assets_cli] Separate out json_syntax_generator into separat…
a3aad77 
…e package (#2101)

Bug: #1826

This refactors `package:hook`s `tool/generate_syntax.dart`.

* Moved into its own package `package:json_syntax_generator`.
* Documented the feature set: optional/required fields, subclassing, tagged unions, and enums.
* Refactored from a single-pass code generator, to a two step pipeline. (1) analyze the JSON schema and extra into a data structure, (2) generate code from the data structure.
* Sprinkled comments about the "why" on the most important classes and fields.

This PR is intended to be a NOP w.r.t. the syntax that's generated. (Some whitespace and `toString` changes.)

Having this as a separate package will enable us to reuse it for the syntax in different places. For now, we're not intending to publish it, instead we can use it as a git dependency in the dev dependencies (or we publish it unlisted). @mosuem Please let me know if it is possible for you to generate the syntax for the `FontAsset`s.

Open issue:

* The generated code requires a `utils/json.dart`. We should make the generated code be standalone. I'll do that in a follow up PR.
auto-submit bot pushed a commit that referenced this issue Mar 17, 2025
@dcharkes
[native_assets_cli] Make generated syntax files stand on their own (#…
329b975 
…2102)

Bug: #1826

`package:json_syntax_generator` is only reusable if the files are self contained.

_Alternatively, we could make the generator take a config option for a JSON import. However, the downside would be that we would need copy paste the library file to multiple places._

Additionally, this PR cleans up all other uses of the `utils/json.dart` in `package:native_assets_cli`. The only remaining JSON helper is `sortOnKey()` because the asset-extension touches JSON manually and needs to sort the keys. (This could be addressed when addressing #2090.)
This was referenced Mar 18, 2025
Merged
Merged
auto-submit bot pushed a commit that referenced this issue Mar 19, 2025
@dcharkes
[native_assets_cli] Add paths to syntax views (#2107)
543902c 
Bug: #1826

This PR adds the path that was navigated through the JSON to the syntax classes. This makes syntax errors more descriptive: `Unexpected value '123' for 'target_os'. Expected a String.` -> `Unexpected value '123' (int) for 'config.code.target_os'. Expected a String.`.

Because the data asset and code asset extensions don't know the internals of the base protocol extension, the extension points now expose both a `json` and a `jsonPath`. That path is then used to instantiate the syntax view of the extension, so it can give more precise error messages.

* For `CodeConfig` this comes naturally, it's a clean view.
* For `EncodedAsset` this is somewhat weird because it is half-used as value class: #2045

(The next PR will add a full syntax validation pass, at which point we should be able to address #2039.)
This was referenced Mar 20, 2025
Merged
Merged
Merged
@auto-submit auto-submit bot closed this as completed in 4073532 Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
package:native_assets_cli type-code-health Internal changes to our tools and workflows to make them cleaner, simpler, or more maintainable
Projects
Native Assets
Status: Done
Milestone
Native Assets v1.x
Development

Successfully merging a pull request may close this issue.

[native_assets_cli] Validate conditionally required fields dart-lang/native
1 participant
@dcharkes