Skip to content

Conversation

@chorobin
Copy link
Contributor

@chorobin chorobin commented Apr 19, 2024

  • Introduce RouteLeaves type to only autocomplete leaves
  • We have to change CheckPath as well (yuck, I can't think of a better way)
  • Add some type tests to cover only leaves

@nx-cloud
Copy link

nx-cloud bot commented Apr 19, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 9ad1791. 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.

@chorobin chorobin force-pushed the suggest-only-route-leaves branch from f2220b6 to 3432501 Compare April 19, 2024 20:23
@chorobin chorobin changed the title fix: do not allow leaf nodes as to suggestions fix: only allow leaf nodes as to suggestions Apr 19, 2024
Comment on lines +437 to +438
? [TRoute] extends [never]
? TFail
Copy link
Contributor

@schiller-manuel schiller-manuel Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really necessary?
I removed it, and type tests still work (and it reduces instantiations from 573059 to 572245)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right atm and thats because we're not strictly checking from. But this first checks if the route exists at all and if it does not then fail.

@chorobin chorobin force-pushed the suggest-only-route-leaves branch from 3432501 to 9ad1791 Compare April 19, 2024 21:04
@chorobin
Copy link
Contributor Author

chorobin commented Apr 19, 2024

Before:

> tsc --extendedDiagnostics

Files:                         589
Lines of Library:            38118
Lines of Definitions:        77165
Lines of TypeScript:         13986
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                114743
Symbols:                    191235
Types:                       50725
Instantiations:             564447
Memory used:               228692K
Assignability cache size:    34134
Identity cache size:          3717
Subtype cache size:              0
Strict subtype cache size:       0
I/O Read time:               0.08s
Parse time:                  0.55s
ResolveModule time:          0.12s
ResolveTypeReference time:   0.01s
ResolveLibrary time:         0.02s
Program time:                0.86s
Bind time:                   0.24s
Check time:                  4.26s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  5.36s

After:

> tsc --extendedDiagnostics

Files:                         589
Lines of Library:            38118
Lines of Definitions:        77166
Lines of TypeScript:         13986
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                114762
Symbols:                    200101
Types:                       51716
Instantiations:             567375
Memory used:               233329K
Assignability cache size:    35133
Identity cache size:          3717
Subtype cache size:              0
Strict subtype cache size:       0
I/O Read time:               0.07s
Parse time:                  0.55s
ResolveModule time:          0.11s
ResolveTypeReference time:   0.01s
ResolveLibrary time:         0.02s
Program time:                0.84s
Bind time:                   0.24s
Check time:                  4.27s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  5.35s

@schiller-manuel schiller-manuel merged commit 09a09f1 into main Apr 19, 2024
@schiller-manuel schiller-manuel deleted the suggest-only-route-leaves branch April 19, 2024 21:46
@jdavidferreira
Copy link

jdavidferreira commented Apr 19, 2024

@chorobin @schiller-manuel

I have some routes like the following:

/parent
/parent/child1

After this change, I cannot do this:

<Link to="/parent"> Parent </Link>

Because TS is complaining, and it says that "/parent" is not assignable to the to type. Parent is not a leaf route, but I should be able to navigate to it, I do have a component/page for that route.

@schiller-manuel
Copy link
Contributor

You need to define an index route at /parent/ if you want to navigate to this route

@nklhtv
Copy link
Contributor

nklhtv commented Apr 24, 2024

@chorobin @schiller-manuel
Ouch, that completely destroys the structure of my app.
I've built an app with a route tree similar to the location masking example (without the actual masking)

const routeTree = rootRoute.addChildren([
  photoRoute,
  photosRoute.addChildren([photoModalRoute]),
  indexRoute,
])

where i have a:

  • route similar to photosRoute which renders a long list and an Outlet at the end
  • route similar to photoModalRoute which renders the details on individual element in modal

If i refactor my route tree to something like photosRoute.addChildren([photosIndexRoute, photoModalRoute]) and render my list of items inside photosIndexRoute then i wont be able to show the modal on top of the list of items.

I've tried to open the location masking example in codesandbox and typescripts complain for using <Link to={'/photos'} now

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants