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.

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

feat(biome_deserialize_derive): adding a rest attribute #2757

Merged

Conversation

NicholasLYang
Copy link
Contributor

Summary

We want the ability to put any unknown fields into a catch-all field, like serde's flatten attribute. This allows for parsing a package.json file while preserving the unknown fields.

Discussion here

This is a first draft, I'd love some guidance on the correct naming/API. Perhaps we could add a new option to the unknown_fields container attribute?

Test Plan

Would love some guidance here!

@NicholasLYang NicholasLYang changed the title feat(biome_deserialize_derive): Adding a flatten attribute feat(biome_deserialize_derive): adding a flatten attribute May 7, 2024
@NicholasLYang NicholasLYang force-pushed the feat/add-flatten-to-deserialize-derive branch from 6e9cb25 to ae242f2 Compare May 9, 2024 14:59
@NicholasLYang NicholasLYang marked this pull request as ready for review May 9, 2024 14:59
Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good first shot :)
We also need to update the derive macro documentation.

Have you tested this change with your use case?

@NicholasLYang
Copy link
Contributor Author

To work with my use case, we need to add a catch all type that can implement Deserializable. I was thinking either AnyJsonValue or serde_json::Value. Is there an easy way to construct AnyJsonValue?

@Conaclos
Copy link
Member

Conaclos commented May 13, 2024

To work with my use case, we need to add a catch all type that can implement Deserializable. I was thinking either AnyJsonValue or serde_json::Value. Is there an easy way to construct AnyJsonValue?

Sorry I completely miss your reply.
I took some time today to think about that. I think it is not a good idea of relying on AnyJsonValue or serde_json::Value for two main reasons:

  1. This will leak implementations details. biome_eeserialize was designed as a way of abstracting over any text format (JSON, YAML, ...). By using one of these types we will break this.
  2. It is currently not possible to convert AnyJsonValue into serde_json::Value or the opposite. Of course, we could implement the conversion, however I am unsure of the difficulty of doing this and this requires more work.

I think a better solution could be of keeping the JSON tree around and to update the tree based on the changes made on the deserialized data. This could look like:

JSON text  --parse--> JSON tree  --deserialize--> (JSON tree, structs) --update structs--> (JSON tree, structs) --update tree--> JSON tree  --print-->  JSON text

I think, for now, this represents too much work.
Thus, I thought about another approach:

We could keep the idea of the flatten attribute and provide generic structures to represent any data. We could have this kind of structure:

enum AnyValue {
  Null,
  Bool(bool),
  Number(TextNumber),
  Str(Text),
  Array(Vec<GenericValue>),
  Map(HshMap<Text, GenericValue>),
}

The disadvantage of this approach is that we deserialize data we don't care about. However, this should be enough to cover your use case for now. What do you think?

@NicholasLYang
Copy link
Contributor Author

Hmm, I was thinking it's okay to implement Deserializable on serde_json::Value, since that's what serde allows as well, even though serde also abstracts over the specific format. While the type is JSON specific, there's nothing that constrains the format that it's being deserialized from. Like if I implement Deserialize on a PackageJson type, obviously it's meant to be deserialized from JSON, but I could deserialize it from YAML too, even though that wouldn't make much sense.

Ultimately though it's up to you! I have a PR that implements Deserializable on serde_json::Value. But if you wish to make a more generic value I can switch it to that.

I think a better solution could be of keeping the JSON tree around and to update the tree based on the changes made on the deserialized data.

We actually do have a limited version of the in place tree modification in the turborepo codebase. And ofc that would be doable with rowan/immutable trees, but agreed that this is out of scope.

@Conaclos
Copy link
Member

Conaclos commented May 14, 2024

Ultimately though it's up to you! I have a PR that implements Deserializable on serde_json::Value. But if you wish to make a more generic value I can switch it to that.

serde_json::Value seems "generic". Thus, I think it is fine. Feel free to submit your other PR.

@NicholasLYang NicholasLYang changed the title feat(biome_deserialize_derive): adding a flatten attribute feat(biome_deserialize_derive): adding a rest attribute May 14, 2024
@NicholasLYang
Copy link
Contributor Author

@Conaclos, any more feedback? I wrote up a little documentation on the attribute

Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks!

@Conaclos Conaclos merged commit 87922b3 into biomejs:main May 15, 2024
10 checks passed
@NicholasLYang
Copy link
Contributor Author

@Conaclos, thanks so much! Do you think it would be possible to do a release for the crates?

@Conaclos
Copy link
Member

Conaclos commented May 24, 2024

thanks so much! Do you think it would be possible to do a release for the crates?

Sure. I'll have a look later today.

@Conaclos
Copy link
Member

@NicholasLYang

I published biome_deserialize@0.6.0 and biome_deserialize_macros@0.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants