It's not must, but FocusManager, OnlineManager is class. so I thought that NotifyManager should be class consistently

Aug 21 '24 14:08 manudeli

☁️ Nx Cloud Report

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

🟥 Failed Commands
`nx affected --targets=test:sherif,test:knip,test:eslint,test:lib,test:types,test:build,build --parallel=3`

✅ Successfully ran 1 target

nx run-many --target=build --exclude=examples/** --exclude=integrations/**

Sent with 💌 from NxCloud.

Aug 21 '24 14:08 nx-cloud[bot]

Open in Stackblitz

More templates

@tanstack/eslint-plugin-query

pnpm add https://pkg.pr.new/@tanstack/eslint-plugin-query@7935

@tanstack/angular-query-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-experimental@7935

@tanstack/query-async-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-async-storage-persister@7935

@tanstack/query-broadcast-client-experimental

pnpm add https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@7935

@tanstack/query-core

pnpm add https://pkg.pr.new/@tanstack/query-core@7935

@tanstack/query-devtools

pnpm add https://pkg.pr.new/@tanstack/query-devtools@7935

@tanstack/query-persist-client-core

pnpm add https://pkg.pr.new/@tanstack/query-persist-client-core@7935

@tanstack/query-sync-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-sync-storage-persister@7935

@tanstack/react-query

pnpm add https://pkg.pr.new/@tanstack/react-query@7935

@tanstack/react-query-devtools

pnpm add https://pkg.pr.new/@tanstack/react-query-devtools@7935

@tanstack/react-query-next-experimental

pnpm add https://pkg.pr.new/@tanstack/react-query-next-experimental@7935

@tanstack/react-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/react-query-persist-client@7935

@tanstack/solid-query

pnpm add https://pkg.pr.new/@tanstack/solid-query@7935

@tanstack/solid-query-devtools

pnpm add https://pkg.pr.new/@tanstack/solid-query-devtools@7935

@tanstack/solid-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/solid-query-persist-client@7935

@tanstack/svelte-query

pnpm add https://pkg.pr.new/@tanstack/svelte-query@7935

@tanstack/svelte-query-devtools

pnpm add https://pkg.pr.new/@tanstack/svelte-query-devtools@7935

@tanstack/svelte-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/svelte-query-persist-client@7935

@tanstack/vue-query

pnpm add https://pkg.pr.new/@tanstack/vue-query@7935

@tanstack/vue-query-devtools

pnpm add https://pkg.pr.new/@tanstack/vue-query-devtools@7935

@tanstack/angular-query-devtools-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@7935

commit: bc88bf4

Aug 21 '24 14:08 pkg-pr-new[bot]

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.85%. Comparing base (0f86b4d) to head (24c0383). Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #7935       +/-   ##
===========================================
+ Coverage   44.51%   61.85%   +17.33%     
===========================================
  Files         195      134       -61     
  Lines        7279     4679     -2600     
  Branches     1629     1304      -325     
===========================================
- Hits         3240     2894      -346     
+ Misses       3662     1543     -2119     
+ Partials      377      242      -135

Components	Coverage Δ
@tanstack/angular-query-devtools-experimental	`∅ <ø> (∅)`
@tanstack/angular-query-experimental	`86.58% <ø> (ø)`
@tanstack/eslint-plugin-query	`∅ <ø> (∅)`
@tanstack/query-async-storage-persister	`43.85% <ø> (ø)`
@tanstack/query-broadcast-client-experimental	`∅ <ø> (∅)`
@tanstack/query-codemods	`∅ <ø> (∅)`
@tanstack/query-core	`92.86% <96.96%> (-0.01%)`	:arrow_down:
@tanstack/query-devtools	`4.86% <ø> (ø)`
@tanstack/query-persist-client-core	`57.73% <ø> (ø)`
@tanstack/query-sync-storage-persister	`82.50% <ø> (ø)`
@tanstack/react-query	`92.50% <100.00%> (ø)`
@tanstack/react-query-devtools	`10.00% <ø> (ø)`
@tanstack/react-query-next-experimental	`∅ <ø> (∅)`
@tanstack/react-query-persist-client	`100.00% <ø> (ø)`
@tanstack/solid-query	`78.20% <ø> (ø)`
@tanstack/solid-query-devtools	`∅ <ø> (∅)`
@tanstack/solid-query-persist-client	`100.00% <ø> (ø)`
@tanstack/svelte-query	`87.33% <ø> (ø)`
@tanstack/svelte-query-devtools	`∅ <ø> (∅)`
@tanstack/svelte-query-persist-client	`100.00% <ø> (ø)`
@tanstack/vue-query	`71.51% <72.72%> (-0.44%)`	:arrow_down:
@tanstack/vue-query-devtools	`∅ <ø> (∅)`

Aug 21 '24 14:08 codecov[bot]

I remember moving this to a function because it was significantly smaller in bundle size. Can you check the size differences please?

Aug 21 '24 14:08 TkDodo

I remember moving this to a function because it was significantly smaller in bundle size. Can you check the size differences please?

Okay, I'll check

Aug 21 '24 14:08 manudeli

Size differences

I don't think it will increase significantly. We can expect library users to minify notifyManager through their bundlers for each service, and if so, we can expect the difference to be even smaller.

AS IS: factory function

TO BE: class with jsdoc

modern: 773,829 bytes -> 775,039 bytes (+1210 bytes) modern/notifyManager.js: 1,534 bytes -> 2,193 bytes (+659 bytes)

JSDoc differences (What I want to focus)

But In my opinion, the biggest difference, other than the size, is that jsdoc in notifyManager is not included when it is a factory function (To include jsdoc, we should attatch jsdoc on return position

function createNotifyManager(){
  /**
   * this jsdoc will be removed on notifyManager when build time
   */
  const batch = <T>(callback: () => T): T => {
    // ...
  }

  // ...

  return {
    /**
     * jsdoc should be on this position, not function definition position
     */
    batch,
    batchCalls,
    schedule,
    setNotifyFunction,
    setBatchNotifyFunction,
    setScheduler,
  } as const
}

). notifyManager is a public API of @tanstack/query-core and I thought it would be a more useful choice to keep the jsdoc where each method is used rather than having it disappear.

AS IS: factory function (jsdoc of methods will be disappeared)

setBatchNotifyFunction or other methods' jsdoc will be removed

TO BE: class (jsdoc of methods will be restored)

setBatchNotifyFunction or other methods' jsdoc will be restored

TO BE: class without jsdoc

modern: 773,829 bytes -> 772,227 bytes (-1602 bytes) modern/notifyManager.js: 1,534 bytes -> 1,739 bytes (+205 bytes)

@TkDodo I'm curious about your opinion on this.

Aug 21 '24 16:08 manudeli

I think we can rather move the jsdoc, or even inline the functions into the return statement:

return {
  batch: <T>(callback: () => T): T => { ... }
}

we also don't really have any meaningful jsdocs here.

Sep 08 '24 16:09 TkDodo

query
query copied to clipboard

fix(query-core): convert notifyManager from factory function to class

☁️ Nx Cloud Report

Codecov Report

Size differences

AS IS: factory function

TO BE: class with jsdoc

JSDoc differences (What I want to focus)

AS IS: factory function (jsdoc of methods will be disappeared)

TO BE: class (jsdoc of methods will be restored)

TO BE: class without jsdoc

query query copied to clipboard

fix(query-core): convert notifyManager from factory function to class

☁️ Nx Cloud Report

Codecov Report

Size differences

AS IS: factory function

TO BE: class with jsdoc

JSDoc differences (What I want to focus)

AS IS: factory function (jsdoc of methods will be disappeared)

TO BE: class (jsdoc of methods will be restored)

TO BE: class without jsdoc

query
query copied to clipboard