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: add support for JsonNode subclasses #2537

Merged
merged 5 commits into from
Jun 25, 2024

Conversation

cromoteca
Copy link
Contributor

Support for JsonNode was added recently. This PR adds the same treatment for its subclasses.

@cromoteca cromoteca requested a review from taefi June 13, 2024 08:32
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.00%. Comparing base (c02ab2e) to head (da9a187).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2537   +/-   ##
=======================================
  Coverage   95.00%   95.00%           
=======================================
  Files          66       66           
  Lines        4546     4546           
  Branches      661      661           
=======================================
  Hits         4319     4319           
  Misses        182      182           
  Partials       45       45           
Flag Coverage Δ
unittests 95.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@platosha
Copy link
Contributor

Just wondering: should we rather represent JsonObject as Record<never, never> and JsonArray as unknown[]?

@cromoteca
Copy link
Contributor Author

I was thinking the same thing, but mostly for simple types like numbers and strings which are transmitted as they are. But I don't expect classes like IntNode to be used as service types, as you can just use int or Integer.
Concerning more complex types like the ones you mentioned, having unknown[] instead of unknown doesn't add big value to me as you still need to cast items. Representing ObjectNode as Record<never, never> shouldn't it be Record<string, unknown>? But then you still have to cast all property values.

@cromoteca
Copy link
Contributor Author

I decided to only support the 3 non-raw-value classes, which also removes the need to use ClassGraph for discovery.

@cromoteca cromoteca requested review from Lodin and platosha June 19, 2024 14:46
Copy link

sonarcloud bot commented Jun 25, 2024

@cromoteca cromoteca merged commit 01d2d55 into main Jun 25, 2024
15 checks passed
@cromoteca cromoteca deleted the feat/support-jsonnode-subclasses branch June 25, 2024 11:22
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Hilla 24.5.0.alpha4 and is also targeting the upcoming stable 24.5.0 version.

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

Successfully merging this pull request may close these issues.

4 participants