-
Notifications
You must be signed in to change notification settings - Fork 56
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 TransferTypes plugin for Generator #3171
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3171 +/- ##
==========================================
+ Coverage 86.74% 86.91% +0.16%
==========================================
Files 120 118 -2
Lines 8196 8194 -2
Branches 1260 1256 -4
==========================================
+ Hits 7110 7122 +12
+ Misses 1071 1058 -13
+ Partials 15 14 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...ertypes/src/main/java/com/vaadin/hilla/parser/plugins/transfertypes/TransferTypesPlugin.java
Show resolved
Hide resolved
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.
Though, changed / added a lot of files, most of the changes were mechanical replacements of types, or changes in the signature of the internal functions, so LGTM!
However, requested changes for the sake of the questions I asked.
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.
Removal of ReadonlyDeep
and addition of the TransferTypes
plugin would have deserved separate PRs. Other changes, like the switch to vitest
in i18n
hardly correlate with this PR.
Yes, I agree with that part. It was just created some issues when I worked with the plugin, so I decided to change it, and my issues were resolved.
Erm... Didn't get it. This PR is about adding that plugin, isn't it?
Agree but CI continued failed because of various weird issues, so to reduce the time spent on debugging these issues, I just decided to do the planned replace. |
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.
I suggest we skip renaming the files unless it's absolutely necessary, since it makes the review process harder. For instance, renaming the packages/ts/frontend/src/types.d.ts
to packages/ts/frontend/src/types.t.ts
had a lot of side effects in many files. Similar for renaming the SharedStorage.d.ts
to SharedStorage.t.ts
.
We can also skip changing from karma
to vitest
, unless it is not working with Karma at all.
4801749
to
eedafbb
Compare
Ok, now there are only changes related to the scope of PR! 😊 |
# Conflicts: # package-lock.json
91c36ee
to
91ff57d
Compare
|
This ticket/PR has been released with Hilla 24.7.0.alpha11 and is also targeting the upcoming stable 24.7.0 version. |
No description provided.