🐷 Implement lazy versions of Path and ArrayPath #7311

felixschorer · 2021-12-17T09:41:16Z

Implements lazy versions of Path and ArrayPath as suggested in #7290

The project does NOT make use of these types yet. They just exist for now. Integrating them into the API is subject of another PR.

Extract path related types into their own submodule (types/utils.ts -> types/path/common.ts / types/path/eager.ts)
Implement primitive types to manipulate paths
Implement lazy alternatives to eager types (Path -> LazyPath, ArrayPath -> LazyArrayPath)
Type tests
- Introduce tsd for type testing
- Integrate tsd into the build pipeline
- Test that the types behave as expected on unions
- Test that the types behave as expected on optional/nullable fields

`LazyPath` and `LazyArrayPath`

Both types validate paths (e.g. 'foo.bar.42.baz') against a type and return a union of suggestions which may include the given path if it is valid. They're supposed to be used on function arguments.

before

function foo<T, P extends Path<T>>(value: T, path: P) {}

after

function foo<T, P extends string>(value: T, path: LazyPath<T, P>) {}

By using those types on function arguments, the path will be validated against T. I.e. TS will report an error when the path does not exist within T. Also, only the immediate successor/child paths will be suggested to the user when typing. Previously Path would report all possible paths, resulting in bad DX for large types.

Typescript below version 4.5

The new types are implemented tail recursively which allows TS to do some optimizations on them. I've tested the types under TS 4.3 and TS 4.4 to make sure that these optimizations aren't needed for the types to work properly. There is also no noticeable difference in performance.
Index signatures using template literal types aren't supported in TS 4.3. The test cases explicitly testing that feature fail under TS 4.3.

IntelliSense

WebStorm and VS Code will show the path suggestions in a dropdown while typing.

Should a path be rejected, neither (WS or VSC) will show the expanded union of suggested paths when hovering over the error.
The error message will some times include "Did you mean ...?", but not always.

WebStorm

VS Code

Import changes

Since a few types have been moved to different files, there may be breaking changes for users which imported these types by their full path.

// ❌ breaks
import { Path } from 'react-hook-form/dist/types/utils'

// ✔ fine
import { Path } from 'react-hook-form'

This could be solved with a re-export in types/utils.ts.

export type {
  ArrayPath,
  FieldArrayPath,
  FieldArrayPathValue,
  FieldPath,
  FieldPathValue,
  FieldPathValues,
  Path,
  PathValue,
} from './path/eager'

felixschorer · 2021-12-17T14:36:46Z

@bluebill1049 I've implemented the types LazyFieldPath and LazyFieldArrayPath now.

I am a fairly new to react-hook-form. Is there anything in particular that I should type test?
These are the test cases that I would implement:

LazyFieldPath for different TFieldValues (object, tuple, array, primitive) and nesting levels (0, 1, 2)
LazyFieldArrayPath for different TFieldValues (object, tuple, array, primitive) and nesting levels (0, 1, 2)
FieldPath should be assignable to LazyFieldPath
FieldArrayPath should be assignable to LazyFieldArrayPath

Seeing that there aren't any comments outside of test cases, how should I document those new types?
I'd be happy to write docstrings in case something isn't clear enough.

barrymay · 2021-12-17T22:38:58Z

Hi @felixschorer this is great stuff, thank you! Actually the more comments the merrier on these types, so please from the top of each add a jsdoc explanation (I would like to do them for all of them, but at minimum your new ones you added).

Separately question: did you see this in another codebase? They look different to me from the ones we saw in ts-toolbelt..

bluebill1049 · 2021-12-17T23:48:31Z

Honestly, this is really impressive! ✨

I am a fairly new to react-hook-form. Is there anything in particular that I should type test?

💯 Would be great to have some test coverage for those types.

Seeing that there aren't any comments outside of test cases, how should I document those new types?
I'd be happy to write docstrings in case something isn't clear enough.

Yes, please add comments. That would be really helpful.

Additional: We also have a lot of integration tests (unit/automation) as well, so probably would catch those issues early as well once you start applying those lazy paths look up.

src/types/path/index.ts

felixschorer · 2021-12-18T02:03:39Z

Separately question: did you see this in another codebase? They look different to me from the ones we saw in ts-toolbelt..

@barrymay I wrote those types from scratch.

I didn't want to rip any code from ts-toolbelt
The requirements are slightly different
It was easier for me to write it from scratch than trying to understand the implementation of AutoPath

I'm very familiar with TS, this is fun for me
The concept of validating a type instead of constraining it is not new to me
Knowing that and having used AutoPath before, it is immediately obvious how it works

TypeScript supports tail call optimization for conditional types since 4.5

Are you implying that I stole that code from somewhere?

bluebill1049 · 2021-12-18T02:17:56Z

Are you implying that I stole that code from somewhere?

@felixschorer I am pretty sure barry didn't mean that. I think he was just surprised with all the types that you have written so far.

felixschorer · 2021-12-18T02:20:44Z

Are you implying that I stole that code from somewhere?

@felixschorer I am pretty sure barry didn't mean that. I think he was just surprised with all the types that you have written so far.

No offense taken. I am not a native speaker, so I may have interpreted it differently than how it was meant. It's also the middle of the night 😅

bluebill1049 · 2021-12-18T02:22:22Z

Indeed I am appreciating (so does the community) the contribution and work that you have been putting this. It's going to be a huge improvement!

barrymay · 2021-12-18T03:19:35Z

Are you implying that I stole that code from somewhere?

Not at all! Just wanted to know, since it is indeed quite a bit of new code, so if it was used elsewhere it would actually give a bit of comfort that it was already "battle tested".
the very last thing I ever want to do is offend anyone helping on this project - any contribution is extremely appreciated and valuable, so thank you!
Since these are new type defs it would be helpful to see some behavior test cases with them in some way if possible.

Again, thank you so much for the work - as Bill said, it's a huge benefit to the project!!

felixschorer · 2021-12-18T15:31:46Z

@bluebill1049 Would it be possible to add a type assertion library as a dev dependency? Testing for type equality is sadly not that trivial due to a lot of edge cases. I'd rather use a library for that instead of rolling my own.

conditional-type-checks worked very well for me so far. I am open for suggestions though.

Also, which version of yarn are you using? My installed version tries to migrate the yarn.lock file to a newer version everytime I run it.

barrymay · 2021-12-18T17:25:02Z

@felixschorer have you checked out https://github.com/SamVerschueren/tsd? I've heard quite a number of refs to it and I was hoping to consider that route (it was also mentioned by a Microsoft contact we heard from a few days ago)

It's somewhat of an extra test step but I feel well worth considering

barrymay · 2021-12-18T17:25:43Z

All that said https://www.npmjs.com/package/conditional-type-checks is quite nice as a simple checking ts lib and on its face seems perfect.

My only concern is whether it's getting actively maintained, but if it works fine and is simple enough Im fine with it.

felixschorer · 2021-12-18T17:42:15Z

@barrymay I have tsd on my radar. I actually mentioned it in my comment but then removed it again in the edit.

I haven't used it yet, but from what I've seen you need to run a script to execute the type tests instead of getting compile time errors. It also seems to be awkward that one has to create a value first.

Honestly, I didn't want to mess with build pipeline 😅

barrymay · 2021-12-18T17:50:06Z

Ha - testing ts types is awkward to start with, so I totally get it! I'd be thrilled if you took Tsd for a spin and see what you thought, it's hard to overlook it, just haven't personally had time to integrate yet.

Considering the (awesome) new types your introducing here, I think it's a perfect time to try it.

bluebill1049 · 2021-12-18T23:05:36Z

thanks, @barrymay's input. I just quickly checked both they are both actively maintained, and syntax wise both are quite similar too. Consider we don't have a type assertion library at the moment. I am keen to take either onboard. Another great add-on we always talked about but never implemented. Really exciting stuff.

…evaluate-paths # Conflicts: # CONTRIBUTING.md # yarn.lock

src/__tests__/types/path/common.test-d.ts

bluebill1049 · 2021-12-24T02:16:20Z

src/__tests__/types/path/common.test-d.ts

+  }
+
+  /** it should work on unions */ {
+    const actual = _ as SplitPathString<'foo.bar' | 'bar.foo'>;


oh nice! union works as well

They all work on unions 😅

bluebill1049 · 2021-12-24T02:26:25Z

src/__tests__/types/path/common.test-d.ts

+    const actual = setter('foo', 'bar');
+    expectType<string>(actual);
+  }
+}


at least 100 plus tests for all the types! impressive ✨ wish we had this earlier.

It should be closer to 200 I think. It was 182 last time I've checked.

Edit: Checked again 197 (in total across all files)

…hCheckConstraint

barrymay · 2021-12-25T03:30:29Z

This is truly an amazing piece of TS work along with great tsd testing and superb jsdoc comments. I need a couple days to finalize my review but this change makes RHF so much stronger.

That said @bluebill1049 if you want to put a beta out with this code so ppl can check it out I'm fine with that.

bluebill1049 · 2021-12-25T05:47:13Z

Thanks, @barrymay sorry to take a bit longer to review this. If you are happy with this PR I can merge it into the branch and publish a beta first.

(In addition, i was hoping to hear some feedback from Kotaro as well, but happy to merge first as it's holiday season right now) :)

felixschorer · 2021-12-25T05:51:04Z

@bluebill1049 @barrymay This PR doesn't change the function definitions yet. It just adds the types. They are currently unused, so releasing a beta is a bit pointless 😅

felixschorer · 2021-12-25T06:15:05Z

To keep the individual PRs manageable (this one is already too large imo) my plan would be to split the work like this:

Add lazy types, but don't use them yet (this PR)
Refactor eager types to be up to standard with the lazy types and ensure compatability between them
Integrate the lazy types into the function signatures
(Fix PathValue being used for function arguments, i.e. setters, as it's distributive on the path when it instead should intersect the types)

bluebill1049 · 2021-12-25T06:18:12Z

sounds good to me @felixschorer I will merge this one in first, I will keep reading the code and probably asking you questions. at this can unblock the rest of the feature.

* 🐷 Implement lazy versions of Path and ArrayPath (#7311) * Extract path related types from types/utils.ts * Implement types for splitting and joining paths * Extract KeyListValue from PathValue * Rename KeyListValue to EvaluateKeyList * Implement Keys to get the keys of a container whose value is U * Add Traversable as the opposite of Primitive * Implement LazyPath and LazyArrayPath * Implement LazyFieldPath and LazyFieldArrayPath * Move Keys & Rename PreviousSuggestion and NextKeySuggestions * Differentiate between AsKey and ToKey * Remove unnecessary wrapper types with default types * Add doc strings for internal types * Add doc strings for lazy types * Use consistent wording * Add doc strings for eager types * Add tsd * Exclude src/*.d-test.ts and src/*.d-test.tsx from TS compilation * Revert "Exclude src/*.d-test.ts and src/*.d-test.tsx from TS compilation" This reverts commit ebedf7f. * Revert "Add tsd" This reverts commit 1546e35. * Add ts-expect * Add expectTypesEqual helper * Add type tests for internal types * Extract EvaluateKey from EvaluateKeyList * Group tests by type * Export internal types to test them * Rename CompletePathString to SuggestPath * Add tests for lazy paths * Test that ValidKeyListPrefix is tail recursive * Use tsd for type tests * Exclude type test files from compilation * Remove Expected type alias for better error messages from tsd * Remove calls to describe / it * Use type assertion instead of call to type<>() * Fix TS failing to infer the PathString in certain cases * Replace default type parameters with Impl types * Make AutoCompletePath distinct from SuggestPaths * Simplify SuggestChildPaths type * Remove Actual type alias for better error reporting from tsd * Add type tests for eager types * Fix ArrayPath tests * Fix LazyArrayPath accepting primitive arrays and tuples * Fix EvaluateKey and EvaluateKeyList not behaving as expected on unions * Fix EvaluateKey handling tuples not correctly * Fix TS2589 for SuggestChildPaths * Fix EvaluateKeyList causing TS2589 * Fix EvaluateKeyList converting any to never * Change EvaluateKeyList to resolve to undefined whether a path does not exist * WIP Fix Keys not working correctly on unions * Fix Keys & add tests * Move test case from Keys to ObjectKeys * Add default for constraint type for ObjectKeys and NumericKeys * Rename KeyList to PathTuple * Add defaults for constraint types * Move Keys to common.ts * Return never from EvaluatePath if a path doesn't exist * Test EvaluateKey and EvaluatePath with index signatures * Fix EvaluatePath resulting in huge types * Fix WS burning CPU cycles * Use HasKey instead of conditional * Add tests for HasPath * Inline DropLastElement and add tests * Test that Suggest* work as expected on unions * Don't suggest paths when a union is passed to AutoCompletePath * Fix Keys omitting optional keys * Test that EvaluateKey can handle optional properties * Fix Keys returning never for nullable types * Fix type tests * Fix Keys constraint matching optional properties when undefined is excluded * Move ContainsIndexable definition closer to where it's used * Fix handling of null in Evaluate* * Fix EvaluateKey not traversing through array methods * Remove unnecessary optimization * Fix child paths not being suggested if they are nullable * Remove unnecessary never check * Fix LazyArrayPath not accepting nullable arrays * Simplify NumericObjectKeys * Rename TupleKey to TupleKeys * Test that EvaluateKey handles null and undefined * Revert PathValue to its former definition * Run API Extractor * Add tests for handling any * Fix blank keys being accepted * Add missing doc string for AppendNonBlankKey * Remove S flag from AutoCompletePathImpl by extracting AutoCompletePathCheckConstraint * Amend comment on AutoCompletePathCheckConstraint * Fix AutoCompletePath accepting malformed paths * Test that AutoCompletePath behaves correctly for never and any * Remove unnecessary check for empty string * update lock * udpdate API extractor * udpdate API extractor * udpdate API extractor * avoid md file with prettier * 💻 Feature/avoid omit key name use field array (improve DX) (#7301) * skip omit keyName when value is presented * fix variable name * remove all ts-ignore * wrap validate inside field check * update deps * revert change on package.json * remove lazy path Co-authored-by: Felix Schorer <felixschorer@users.noreply.github.com>

felixschorer added 9 commits December 17, 2021 10:38

Extract path related types from types/utils.ts

d0cb388

Implement types for splitting and joining paths

4f84614

Extract KeyListValue from PathValue

e8dfc8b

Rename KeyListValue to EvaluateKeyList

0af8768

Implement Keys to get the keys of a container whose value is U

7ba3cf3

Add Traversable as the opposite of Primitive

3013aee

Implement LazyPath and LazyArrayPath

02c758d

Implement LazyFieldPath and LazyFieldArrayPath

81f5653

Move Keys & Rename PreviousSuggestion and NextKeySuggestions

d3f6b6d

bluebill1049 reviewed Dec 17, 2021

View reviewed changes

src/types/path/index.ts Outdated Show resolved Hide resolved

felixschorer added 6 commits December 18, 2021 11:01

Differentiate between AsKey and ToKey

485fd13

Remove unnecessary wrapper types with default types

6e31647

Add doc strings for internal types

706054a

Add doc strings for lazy types

7f78f07

Use consistent wording

58a9a62

Add doc strings for eager types

fde7540

felixschorer added 4 commits December 23, 2021 09:16

Test that EvaluateKey handles null and undefined

a0c231a

Revert PathValue to its former definition

0c5ba00

Merge remote-tracking branch 'origin/master' into enhancement/lazily-…

63fc63e

…evaluate-paths # Conflicts: # CONTRIBUTING.md # yarn.lock

Run API Extractor

219bdd0

felixschorer changed the title ~~Enhancement/Lazily evaluate paths to speed up TypeScript~~ Implement lazy versions of Path and ArrayPath Dec 23, 2021

felixschorer marked this pull request as ready for review December 23, 2021 10:43

felixschorer requested review from barrymay and bluebill1049 December 23, 2021 10:43

bluebill1049 requested a review from kotarella1110 December 23, 2021 10:51

Add tests for handling any

e39742b

bluebill1049 changed the title ~~Implement lazy versions of Path and ArrayPath~~ 🐷 Implement lazy versions of Path and ArrayPath Dec 23, 2021

bluebill1049 reviewed Dec 24, 2021

View reviewed changes

src/__tests__/types/path/common.test-d.ts Show resolved Hide resolved

bluebill1049 reviewed Dec 24, 2021

View reviewed changes

felixschorer added 7 commits December 24, 2021 11:32

Fix blank keys being accepted

b6ca97f

Add missing doc string for AppendNonBlankKey

f948b90

Remove S flag from AutoCompletePathImpl by extracting AutoCompletePat…

8a51267

…hCheckConstraint

Amend comment on AutoCompletePathCheckConstraint

4124154

Fix AutoCompletePath accepting malformed paths

dfd23eb

Test that AutoCompletePath behaves correctly for never and any

8a6b7d3

Remove unnecessary check for empty string

b3845d9

bluebill1049 merged commit 93a7b89 into react-hook-form:7.23.0 Dec 25, 2021

bluebill1049 mentioned this pull request Dec 25, 2021

Version 7.23.0 (next) #7366

Merged

2 tasks

felixschorer deleted the enhancement/lazily-evaluate-paths branch December 25, 2021 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐷 Implement lazy versions of Path and ArrayPath #7311

🐷 Implement lazy versions of Path and ArrayPath #7311

felixschorer commented Dec 17, 2021 •

edited

Loading

felixschorer commented Dec 17, 2021

barrymay commented Dec 17, 2021 •

edited

Loading

bluebill1049 commented Dec 17, 2021 •

edited

Loading

felixschorer commented Dec 18, 2021 •

edited

Loading

bluebill1049 commented Dec 18, 2021

felixschorer commented Dec 18, 2021

bluebill1049 commented Dec 18, 2021

barrymay commented Dec 18, 2021 •

edited

Loading

felixschorer commented Dec 18, 2021 •

edited

Loading

barrymay commented Dec 18, 2021 •

edited

Loading

barrymay commented Dec 18, 2021 •

edited

Loading

felixschorer commented Dec 18, 2021

barrymay commented Dec 18, 2021

bluebill1049 commented Dec 18, 2021

bluebill1049 Dec 24, 2021

felixschorer Dec 24, 2021

bluebill1049 Dec 24, 2021

felixschorer Dec 24, 2021 •

edited

Loading

barrymay commented Dec 25, 2021 •

edited

Loading

bluebill1049 commented Dec 25, 2021 •

edited

Loading

felixschorer commented Dec 25, 2021 •

edited

Loading

felixschorer commented Dec 25, 2021 •

edited

Loading

bluebill1049 commented Dec 25, 2021

🐷 Implement lazy versions of Path and ArrayPath #7311

🐷 Implement lazy versions of Path and ArrayPath #7311

Conversation

felixschorer commented Dec 17, 2021 • edited Loading

LazyPath and LazyArrayPath

Typescript below version 4.5

IntelliSense

Import changes

felixschorer commented Dec 17, 2021

barrymay commented Dec 17, 2021 • edited Loading

bluebill1049 commented Dec 17, 2021 • edited Loading

felixschorer commented Dec 18, 2021 • edited Loading

bluebill1049 commented Dec 18, 2021

felixschorer commented Dec 18, 2021

bluebill1049 commented Dec 18, 2021

barrymay commented Dec 18, 2021 • edited Loading

felixschorer commented Dec 18, 2021 • edited Loading

barrymay commented Dec 18, 2021 • edited Loading

barrymay commented Dec 18, 2021 • edited Loading

felixschorer commented Dec 18, 2021

barrymay commented Dec 18, 2021

bluebill1049 commented Dec 18, 2021

bluebill1049 Dec 24, 2021

Choose a reason for hiding this comment

felixschorer Dec 24, 2021

Choose a reason for hiding this comment

bluebill1049 Dec 24, 2021

Choose a reason for hiding this comment

felixschorer Dec 24, 2021 • edited Loading

Choose a reason for hiding this comment

barrymay commented Dec 25, 2021 • edited Loading

bluebill1049 commented Dec 25, 2021 • edited Loading

felixschorer commented Dec 25, 2021 • edited Loading

felixschorer commented Dec 25, 2021 • edited Loading

bluebill1049 commented Dec 25, 2021

felixschorer commented Dec 17, 2021 •

edited

Loading

`LazyPath` and `LazyArrayPath`

barrymay commented Dec 17, 2021 •

edited

Loading

bluebill1049 commented Dec 17, 2021 •

edited

Loading

felixschorer commented Dec 18, 2021 •

edited

Loading

barrymay commented Dec 18, 2021 •

edited

Loading

felixschorer commented Dec 18, 2021 •

edited

Loading

barrymay commented Dec 18, 2021 •

edited

Loading

barrymay commented Dec 18, 2021 •

edited

Loading

felixschorer Dec 24, 2021 •

edited

Loading

barrymay commented Dec 25, 2021 •

edited

Loading

bluebill1049 commented Dec 25, 2021 •

edited

Loading

felixschorer commented Dec 25, 2021 •

edited

Loading

felixschorer commented Dec 25, 2021 •

edited

Loading