perf(form-core): reduce instantiations when type checking #1262
Conversation
|
View your CI Pipeline Execution ↗ for commit ea07bce.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1262 +/- ##
==========================================
- Coverage 88.88% 88.86% -0.02%
==========================================
Files 28 28
Lines 1277 1275 -2
Branches 335 332 -3
==========================================
- Hits 1135 1133 -2
Misses 126 126
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…e annotations and remove Omit
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
| @@ -24,8 +24,13 @@ assertType< | |||
| type ArraySupport = DeepKeys<{ users: User[] }> | |||
| assertType< | |||
| | 'users' | |||
| | `users[number]` | |||
There was a problem hiding this comment.
Question: the types with [number] are added to ArrayAccessor on purpose for the autocomplete, right?
I see it goes from this (on main)
to this
There was a problem hiding this comment.
Yes, what do you think about this? Would you prefer it is not here?
There was a problem hiding this comment.
Let's talk about it.
At the current state you don't get any suggestion. With this change we're showing/accepting a value that is technically wrong (people[number].age does not exist and will throw an error at runtime) but points to the right direction, showing an example of what you can do.
The runtime error might be annoying as it's not much self explanatory, I tried and I got Uncaught TypeError: field.state.value.map is not a function only after editing an item of an array.
I don't know, since it's just a type change we might even let this in and remove it later if people get more confused than helped but in my case at first I thought it was an error, then I realized it was probably a nice hint.
@crutchcorn and @harry-whorlow what do you think?
There was a problem hiding this comment.
Hm, yeah. Thinking about it. I don't think this is a good idea to have for now. I think ideally it should show as a suggestion but then type error if you actually use it. But this then requires the user to edit the type after they have selected it which I'm not sure if I like
There was a problem hiding this comment.
Is this something that we've seen requested? So far to me it seems like potentially adding complexity in a situation where the benefit isn't that apparent... But either way I don't have a strong opinion. 😊
…es' into perf-annotations-deep-key-values
…es' into perf-annotations-deep-key-values
Balastrong
force-pushed
the
perf-annotations-deep-key-values
branch
from
8efda0a to
3a3de2e
Compare
|
I'm merging it to avoid conflicts on long lived branches, thanks again! |
) * perf: add variance annotations * perf(form-core): improve ts performance for large example * ci: apply automated fixes and generate docs * perf(form-core): rework to use UnionToIntersection, add other variance annotations and remove Omit * ci: apply automated fixes and generate docs * fix: revert some variance annotations for later * ci: apply automated fixes and generate docs * chore: fix knip * chore: remove omit * ci: apply automated fixes and generate docs * perf(solid-form): apply interface performance improvments to solid * ci: apply automated fixes and generate docs * perf(vue-form): add interface changes to vue * ci: apply automated fixes and generate docs * chore: cleanup * ci: apply automated fixes and generate docs * chore: cleanup * ci: apply automated fixes and generate docs * refactor: use mapped type instead of union to intersection * ci: apply automated fixes and generate docs * chore: fix build * ci: apply automated fixes and generate docs * chore: change back to 'name' * ci: apply automated fixes and generate docs * fix: remove [number] * ci: apply automated fixes and generate docs
I have used the large example in the repo (this isn't very large so difference is not so large) to check reduction in instantiations and type checking time. I am using
extendedDiagnosticsbut also traced the exampleBefore:
After
There are three main things contributing to lower instantiations:
TNameis not constrained and uses aValidateNameutility instead. I prefer not to use contraints at inference sites against complex types that could be unions. This is becauseTNamecould default to the union and this can slow things down in a few situations.DeepKeysandDeepValue. Instead of creating a union and then parsingTNameagain, a map is constructed soTValuecan be grabbed byTName.TName->TValue