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

manudeli · 2024-08-21T14:06:23Z

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

nx-cloud · 2024-08-21T14:13:27Z

☁️ Nx Cloud Report

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

pkg-pr-new · 2024-08-21T14:15:26Z

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

codecov · 2024-08-21T14:20:17Z

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%)`	⬇️
@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%)`	⬇️
@tanstack/vue-query-devtools	`∅ <ø> (∅)`

TkDodo · 2024-08-21T14:28:20Z

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

manudeli · 2024-08-21T14:37:28Z

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

manudeli · 2024-08-21T16:01:08Z

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.

TkDodo · 2024-09-08T16:28:57Z

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.

github-actions bot added the package: query-core label Aug 21, 2024

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

47181a1

manudeli force-pushed the query-core/fix/notifyManager branch from fc5b2de to 47181a1 Compare August 21, 2024 14:07

manudeli marked this pull request as ready for review August 21, 2024 14:11

manudeli added 14 commits August 24, 2024 13:51

Merge branch 'main' into query-core/fix/notifyManager

f85d6de

Merge branch 'main' into query-core/fix/notifyManager

797d32c

Merge branch 'main' into query-core/fix/notifyManager

f520e2b

Merge branch 'main' into query-core/fix/notifyManager

7522e57

Merge branch 'main' into query-core/fix/notifyManager

3982fb1

Merge branch 'main' into query-core/fix/notifyManager

8f9aa54

Merge branch 'main' into query-core/fix/notifyManager

3cb8469

Merge branch 'main' into query-core/fix/notifyManager

e4a662a

Merge branch 'main' into query-core/fix/notifyManager

88063ef

Merge branch 'main' into query-core/fix/notifyManager

4a96b6d

Merge branch 'main' into query-core/fix/notifyManager

f3b3bc1

Merge branch 'main' into query-core/fix/notifyManager

53080d7

Merge branch 'main' into query-core/fix/notifyManager

24c0383

Merge branch 'main' into query-core/fix/notifyManager

bc88bf4

TkDodo closed this Sep 8, 2024

manudeli mentioned this pull request Sep 8, 2024

fix(query-core): jsdoc of notifyManager #8031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

manudeli commented Aug 21, 2024 •

edited

Loading

nx-cloud bot commented Aug 21, 2024 •

edited

Loading

pkg-pr-new bot commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

TkDodo commented Aug 21, 2024

manudeli commented Aug 21, 2024

manudeli commented Aug 21, 2024 •

edited

Loading

TkDodo commented Sep 8, 2024

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

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

Conversation

manudeli commented Aug 21, 2024 • edited Loading

nx-cloud bot commented Aug 21, 2024 • edited Loading

☁️ Nx Cloud Report

pkg-pr-new bot commented Aug 21, 2024 • edited Loading

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

TkDodo commented Aug 21, 2024

manudeli commented Aug 21, 2024

manudeli commented Aug 21, 2024 • edited Loading

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

TkDodo commented Sep 8, 2024

manudeli commented Aug 21, 2024 •

edited

Loading

nx-cloud bot commented Aug 21, 2024 •

edited

Loading

pkg-pr-new bot commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

manudeli commented Aug 21, 2024 •

edited

Loading