tests: fix type tests for Link, make sure type tests fail in ci and add type tests for union `to` and difference in params #1418

chorobin · 2024-04-02T18:13:49Z

Tests were failing and were not type checking in ci
Added test cases for to being a union, params being optional if from and to contain the same params

…dd type tests for union `to` and difference in params

…-tests

nx-cloud · 2024-04-02T18:14:17Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e58250f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

schiller-manuel · 2024-04-02T19:22:36Z

packages/react-router/tests/link.test-d.tsx

+  params.parameter(0).toEqualTypeOf<{ invoiceId: string }>()
+})
+
+test('when navigating to a union of routes, params is optional', () => {


is this correct though? shouldn't params be { invoiceId: string } | { postId: string}`?

yeah I was expecting this test:

https://github.com/TanStack/router/pull/1418/files#diff-921603b68301ae5e52534582aeb75f7defc2946a96ad3b268b28ad34ae5fec04R262

why would we need this test here? the test below already checks everything, right?

I will double check but I believe this is slightly different as we're checking if it's optional. But it's a good point if this is intentional behaviour or not. Should it be optional for a union of different routes?

I find the unions a harder use case to think about if it should be optional or not. For example the default case string falls under the same code path and this makes sense to be optional

string is basically considered to be any route and is therefore a union of everything

yeah I was expecting this test:

https://github.com/TanStack/router/pull/1418/files#diff-921603b68301ae5e52534582aeb75f7defc2946a96ad3b268b28ad34ae5fec04R262

why would we need this test here? the test below already checks everything, right?

Yep. I can explain why there is a seperate test here. The test you're referring too checks if the type of params can be the following (excluding the function and boolean types)

{ invoiceId: string } | { postId: string } | undefined

This is not the same as checking if the property is optional in typescript. A type can be undefined but still required. Therefore I think this check is necessary because we need to ensure its optional explicitly. I'd rather not assume undefined means optional because its not the same.

Please correct me if you think I'm wrong

schiller-manuel · 2024-04-02T19:24:27Z

packages/react-router/tests/link.test-d.tsx

+    string,
+    '/invoices/$invoiceId/' | '/posts/$postId/'
+  >
+  expectTypeOf(TestLink).parameter(0).not.toMatchTypeOf<{ params: unknown }>()


is this the type check for "params is optional"? how? and is there a more explicit way?

Docs cover this

https://github.com/mmkal/expect-type#error-messages

I'm going to fix these comments tomorrow so don't merge. But you have me doubting myself so I will check

I'm trying to think of a different way but it's difficult as the types can be quite complex for params

schiller-manuel · 2024-04-02T19:26:54Z

packages/react-router/tests/link.test-d.tsx

+    .toEqualTypeOf<{} | { invoiceId: string } | { postId: string }>()
+})
+
+test('when navigating to a union of routes including the route, params is optional', () => {


"including the route" .. which one?

ah you meant "the root", right?

Yep. Sorry, bad spelling

schiller-manuel · 2024-04-02T19:30:06Z

packages/react-router/tests/link.test-d.tsx

+  params.parameter(0).toEqualTypeOf<{} | { page: number }>()
+})
+
+test('when navigating to a union of routes including the route, search params is optional', () => {


"the route" => "the root"

Yep. Same mistake!

SeanCassiere

Correct me if I'm wrong, but I believe we aren't testing for external links.

The implementation for the <Link> component checks for external links and sets the href property to whatever the input of to was.

Not sure if that's something that'll be covered by this PR or not, just putting it out there since the types for Link are being touched here.

chorobin · 2024-04-03T16:12:34Z

Correct me if I'm wrong, but I believe we aren't testing for external links.

The implementation for the <Link> component checks for external links and sets the href property to whatever the input of to was.

Not sure if that's something that'll be covered by this PR or not, just putting it out there since the types for Link are being touched here.

Good point. Maybe we can do sanity check that we allow any string literal type in to. But bare in mind these are only type tests and doesn't test the runtime behaviour. It would be good to have a Link.test.tsx file which tests the runtime

We need both but since I'm touching the types most of the time, that's what I see break the most so I wanted to add some.

But yes. I agree we need tests for this scenario aswell

…-tests

There was a lot of repetition in the tests, I think its simpler to group the tests by use cases and have multiple expects for the use case

chorobin added 2 commits April 2, 2024 20:06

tests: fix type tests for Link, make sure type tests fail in ci and a…

6f302e4

…dd type tests for union `to` and difference in params

Merge remote-tracking branch 'origin/main' into f/add-union-link-type…

92d8df8

…-tests

schiller-manuel reviewed Apr 2, 2024

View reviewed changes

SeanCassiere reviewed Apr 3, 2024

View reviewed changes

chorobin added 2 commits April 4, 2024 11:24

Merge remote-tracking branch 'origin/main' into f/add-union-link-type…

5d3b7dc

…-tests

tests: refactor tests to only include scenario and group expects

e58250f

There was a lot of repetition in the tests, I think its simpler to group the tests by use cases and have multiple expects for the use case

schiller-manuel merged commit 03a91d8 into TanStack:main Apr 4, 2024

Uh oh!

tests: fix type tests for Link, make sure type tests fail in ci and add type tests for union to and difference in params #1418

tests: fix type tests for Link, make sure type tests fail in ci and add type tests for union to and difference in params #1418

Uh oh!

Conversation

chorobin commented Apr 2, 2024

Uh oh!

nx-cloud bot commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chorobin Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chorobin Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeanCassiere left a comment

Choose a reason for hiding this comment

Uh oh!

chorobin commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tests: fix type tests for Link, make sure type tests fail in ci and add type tests for union `to` and difference in params #1418

tests: fix type tests for Link, make sure type tests fail in ci and add type tests for union `to` and difference in params #1418

nx-cloud bot commented Apr 2, 2024 •

edited

Loading

chorobin Apr 4, 2024 •

edited

Loading

chorobin Apr 3, 2024 •

edited

Loading

chorobin commented Apr 3, 2024 •

edited

Loading