fix(*): change noop function return type from undefined to void #8299
Conversation
|
didn’t you just literally change the same thing @manudeli ? I’m confused |
@kangju2000 check this comment #8294 (comment) please |
|
Sorry for not checking PR #8294 (comment). What do you think about adding this comment to help future contributors understand the design decision? @TkDodo @manudeli /**
* No-operation function that returns undefined for type safety in Promise chains
*/
function noop(): undefined {
return undefined
} |
|
@TkDodo Since noop returning void and functions returning undefined are widely used in all packages, How about providing both |
|
Quick, simple suggestion: export function noop(): void | undefined {}Is this ok? |
|
That's fine if TypeScript likes it everywhere 👍 |
|
If that makes typescript unhappy, we may just let it be inferred .. export function noop(): {
return undefined
}up to you guys 😅 |
kangju2000
force-pushed
the
refactor/noop
branch
from
93c5d4c
to
0b9e918
Compare
…ement Co-authored-by: Jonghyeon Ko <[email protected]> Co-authored-by: Sol Lee <[email protected]>
Looking at this comment(#8294 (comment)), it seems that TkDodo did not want to implement noop and returnUndefined separately and include them in the package bundle since they are the same implementation in JavaScript. So, how about implementing it as follows? // @tanstack/query-core
export function noop(): void {}
export const returnUndefined = noop as () => undefined // re-assert type
// @tanstack/react-query just use @tanstack/query-core's noop, returnUndefined
import { noop, returnUndefined } from '@tanstack/query-core'
// @tanstack/vue-query just use @tanstack/query-core's noop, returnUndefined
import { noop, returnUndefined } from '@tanstack/query-core'
// @tanstack/svelte-query just use @tanstack/query-core's noop, returnUndefined
import { noop, returnUndefined } from '@tanstack/query-core'if don't want to export noop, returnUndefined in @tanstack/query-core, How about implementing like below // @tanstack/query-core
export function noop(): void {}
export const returnUndefined = noop as () => undefined // re-assert type
// @tanstack/react-query
export function noop(): void {}
export const returnUndefined = noop as () => undefined // re-assert type
// @tanstack/vue-query
export function noop(): void {}
export const returnUndefined = noop as () => undefined // re-assert type
// @tanstack/svelte-query
export function noop(): void {}
export const returnUndefined = noop as () => undefined // re-assert type |
|
Thank you for the good suggestion! From a bundle size optimization perspective, type re-assertion seems better. I'll try changing it to Additionally, these utility functions seem to fall outside the core responsibilities of @tanstack/query-core and would be better handled through dedicated utility libraries. |
packages/query-core/src/utils.ts
Outdated
| export function noop(): void {} | ||
|
|
||
| export const returnUndefined = noop as () => undefined |
There was a problem hiding this comment.
would overloads also work?
export function noop(): undefined
export function noop(): void
export function noop() {}
There was a problem hiding this comment.
Thanks for the suggestion!🙌 reflected in 454956e
There was a problem hiding this comment.
I found an interesting TypeScript behavior.
// Works: No type error when used in Promise.catch()
// Type inference: function noop(): void (+1 overload)
export function noop(): void
export function noop(): undefined
export function noop() {}
// Doesn't work: Type error when used in Promise<TQueryData | undefined>.catch()
// Type inference: function noop(): undefined (+1 overload)
export function noop(): undefined
export function noop(): void
export function noop() {}
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 9a6220d. 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 targetsSent with 💌 from NxCloud. |
|
it doesn’t work. please make sure locally that everything compiles properly. |
|
@kangju2000 You have to run script |
|
Sorry for the hassle. I've added type overrides for noop only where necessary. Using the overridden noop in the following code causes errors, so I kept it unchanged in solid-query. // packages/solid-query/src/createQueries.ts:309
let unsubscribe = noop
createComputed<() => void>((cleanup) => {
cleanup?.()
unsubscribe = isRestoring() ? noop : subscribeToObserver()
return () => queueMicrotask(unsubscribe)
}) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8299 +/- ##
=========================================
+ Coverage 0 62.69% +62.69%
=========================================
Files 0 135 +135
Lines 0 4798 +4798
Branches 0 1351 +1351
=========================================
+ Hits 0 3008 +3008
- Misses 0 1548 +1548
- Partials 0 242 +242
|
I modify the return type of noop function from
() => {}tonoop():voidfor type consistency.[AS-IS]
undefinedwhile methods in query-core returnPromise<void>promise.catch(noop)have inconsistent return types with their declarations#executeFetchmethod has the same issue wherePromise<TQueryData | undefined>meetspromise.catch(noop)[TO-BE]
voidpromise.catch(noop)withpromise.catch(() => undefined)in#executeFetchto match its return typenoop():voidto other promise chains in query-core where return type isPromise<void>