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

perf(react-router): remove root search schema type and unused type parameters from route options #1729

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

chorobin
Copy link
Contributor

@chorobin chorobin commented Jun 9, 2024

  • remove root search schema as it doesn't seem to be needed
  • remove unused type parameters for route options
  • added tests to cover index routes and root search schema

Copy link

nx-cloud bot commented Jun 9, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 7595d66. 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 4 targets

Sent with 💌 from NxCloud.

@chorobin
Copy link
Contributor Author

chorobin commented Jun 9, 2024

Reduces instantiations a bit because we don't need to exclude/omit RootSearchSchema
Before

> tsc --extendedDiagnostics

Files:                         644
Lines of Library:            38411
Lines of Definitions:        82809
Lines of TypeScript:         19639
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                134746
Symbols:                    326768
Types:                      115290
Instantiations:             826289
Memory used:               392053K
Assignability cache size:    47858
Identity cache size:          8641
Subtype cache size:           2426
Strict subtype cache size:     413
I/O Read time:               0.10s
Parse time:                  0.78s
ResolveModule time:          0.16s
ResolveTypeReference time:   0.01s
ResolveLibrary time:         0.02s
Program time:                1.19s
Bind time:                   0.32s
Check time:                  5.96s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  7.47s

After

> tsc --extendedDiagnostics

Files:                         644
Lines of Library:            38411
Lines of Definitions:        82800
Lines of TypeScript:         19639
Lines of JavaScript:             0
Lines of JSON:                   0
Lines of Other:                  0
Identifiers:                134667
Symbols:                    326556
Types:                      115064
Instantiations:             816475
Memory used:               386904K
Assignability cache size:    47482
Identity cache size:          8539
Subtype cache size:           2426
Strict subtype cache size:     413
I/O Read time:               0.08s
Parse time:                  0.59s
ResolveModule time:          0.13s
ResolveTypeReference time:   0.00s
ResolveLibrary time:         0.02s
Program time:                0.92s
Bind time:                   0.27s
Check time:                  5.47s
printTime time:              0.00s
Emit time:                   0.00s
Total time:                  6.65s

@chorobin
Copy link
Contributor Author

chorobin commented Jun 9, 2024

Don't merge just yet. Just want to double check everything

@SeanCassiere
Copy link
Member

Just want to confirm the RootSearchSchema type wasn't being used for Router inference right? So it's getting nuked.

@chorobin
Copy link
Contributor Author

chorobin commented Jun 10, 2024

Just want to confirm the RootSearchSchema type wasn't being used for Router inference right? So it's getting nuked.

From what I understand from conversations with @schiller-manuel the users don't use this type unlike SearchSchemaInput? It seems like there was a bug some time ago confusing the root route and the index route. But this doesn't seem to be a problem anymore. There's been a lot of changes to the types since then so if we don't need it, I'd remove it because it just means we need to check for it all the time

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.

2 participants