close
Skip to content

feat(types): cheaper WithRequired - #4922

Merged
TkDodo merged 4 commits into
TanStack:mainfrom
Andarist:cheaper-with-required
Jul 21, 2023
Merged

feat(types): cheaper WithRequired#4922
TkDodo merged 4 commits into
TanStack:mainfrom
Andarist:cheaper-with-required

Conversation

@Andarist

@Andarist Andarist commented Feb 2, 2023

Copy link
Copy Markdown
Contributor

To the best of my understanding - this should work just fine but I didn't test it "thoroughly" with TanStack Query.

I'm opening this PR mostly because I got curious if it will pass your CI jobs. If it does though - it might be worth merging. This reduces 3 mapped types to just one :)

@vercel

vercel Bot commented Feb 2, 2023

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
query ⬜️ Ignored (Inspect) Jul 21, 2023 9:17am

@codesandbox-ci

codesandbox-ci Bot commented Feb 2, 2023

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit db30de1:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@codecov-commenter

codecov-commenter commented Feb 2, 2023

Copy link
Copy Markdown

Codecov Report

Patch and project coverage have no change.

Comparison is base (5188ae3) 91.79% compared to head (644d223) 91.79%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4922   +/-   ##
=======================================
  Coverage   91.79%   91.79%           
=======================================
  Files         101      101           
  Lines        3890     3890           
  Branches      974      974           
=======================================
  Hits         3571     3571           
  Misses        298      298           
  Partials       21       21           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@DamianOsipiuk

Copy link
Copy Markdown
Contributor

@Andarist Any idea how much faster this one performs?
Or maybe do you have an idea how to measure it?

Seems to be working as intended, but looks weird 😂

@Andarist

Andarist commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

Hard to tell really. It's "theoretically" faster by the sole virtue of using fewer mapped types. The best way to test this would be to run tsc --extendedDiagnostics with both versions in a project that is using this library a lot.

@DamianOsipiuk

Copy link
Copy Markdown
Contributor

So i ran a couple of runs on vue-query which seems to be using more of WithRequired with added diagnostics, and clearing builds between runs.

It seems like it improves the time by ~0.05s and memory usage by ~5000K.

Of course those tests were not very scientific 🧑‍🔬 🔫 But it seems it does make a difference.
I would be curious about performance benefit in a large app.

@TkDodo Should we merge it?

@Andarist

Copy link
Copy Markdown
Contributor Author

It also might be worth checking out the functionality of this code in pre-4.8 - since this is a version that changed some rules around T & {}

@TkDodo

TkDodo commented Feb 22, 2023

Copy link
Copy Markdown
Collaborator

Good point, we want to support 4.7 and onwards in v5

@Andarist

Andarist commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Note that I did recheck this (and everything looks alright) - it's just that I'm advising extra caution because of this TS 4.8 change.

@nx-cloud

nx-cloud Bot commented May 28, 2023

Copy link
Copy Markdown

☁️ Nx Cloud Report

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

📂 See all runs for this branch


🟥 Failed Commands
nx affected --targets=test:eslint,test:lib,test:types,test:build
✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@TkDodo
TkDodo merged commit 456e5ee into TanStack:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants