Skip to content
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

fix: only allow leaf nodes as to suggestions #1495

Merged
merged 1 commit into from
Apr 19, 2024

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

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 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

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
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
7 checks passed
@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.

None yet

4 participants