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

fix(apiWatch): correct type inference for reactive array #11036

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

jh-leong
Copy link
Contributor

close #9416.

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 90.8 kB 34.5 kB 31.1 kB
vue.global.prod.js 148 kB 53.7 kB 48 kB

Usages

Name Size Gzip Brotli
createApp 50.8 kB 19.9 kB 18.1 kB
createSSRApp 54.1 kB 21.2 kB 19.3 kB
defineCustomElement 53.1 kB 20.6 kB 18.8 kB
overall 64.6 kB 24.9 kB 22.6 kB

@jh-leong jh-leong force-pushed the fix/watch-reactive-array branch 3 times, most recently from 98655af to 6ea95a4 Compare May 29, 2024 14:27
@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented May 30, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools success success
nuxt ⏹️ cancelled success
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify undefined skipped success
vueuse failure success
vue-simple-compiler success success

@yyx990803
Copy link
Member

From ecosystem-ci this PR seems to break VueUse's dts build: https://github.com/vuejs/ecosystem-ci/actions/runs/9296270402/job/25584547935#step:7:77

@jh-leong
Copy link
Contributor Author

From ecosystem-ci this PR seems to break VueUse's dts build: vuejs/ecosystem-ci/actions/runs/9296270402/job/25584547935#step:7:77

Fixed now, could you please rerun the ecosystem CI? Thanks :)

@edison1105
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented May 30, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools success success
nuxt ⏹️ cancelled success
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify ⏹️ cancelled success
vueuse failure success
vue-simple-compiler success success

@jh-leong
Copy link
Contributor Author

To address the issue at the type level, reactive array and normal arrays must be differentiated in the watch API type inference.

Currently, the code can't distinguish between them, as shown below:

// Both are inferred as Ref<number>[]
const arr1 = [ref(1)]
const arr2 = reactive(arr1)

To solve this, need to mark reactive array types specially. I considered two ways:

  1. Using a unique symbol
  2. Using a private class

However, both ways introduce breaking changes:

declare const ReactiveMarkerSymbol: unique symbol

export declare class ReactiveMarker {
  private [ReactiveMarkerSymbol]?: void
}

export type Reactive<T> = UnwrapNestedRefs<T> & ReactiveMarker

export type Reactive2<T> = UnwrapNestedRefs<T> & {
  [ReactiveMarkerSymbol]?: void
}

type Foo = { count: number }
declare const vPrivate: Reactive<Foo>
declare const vSymbol: Reactive2<Foo>

{
  let a: { count: number } = vSymbol // ok
  let b: { count: number } = vPrivate // ok
}
{
  let a: Record<string, number> = vSymbol // ok

  // Breaking change: Assigning to a Record (or index signature) will result in an error
  // because the Class can use the Interface for declaration merging
  let b: Record<string, number> = vPrivate // error!
}
{
  // Breaking change: the symbol key leaked when using keyof
  type a = keyof typeof vSymbol // "count" | typeof ReactiveMarkerSymbol

  // ok
  type b = keyof typeof vPrivate // "count"
}
{
  type Vals<T> = T[keyof T]

  // Breaking change: the value of the symbol key is leaked when getting Vals
  type a = Vals<typeof vSymbol> // number | undefined

  // ok
  type b = Vals<typeof vPrivate> // number
}

To avoid these breaking changes, I've decided to mark only reactive arrays specially:

export declare class ReactiveMarker {
  private [ReactiveMarkerSymbol]?: void
}
export type Reactive<T> = UnwrapNestedRefs<T> &
  (T extends readonly any[] ? ReactiveMarker : {})

Due to the special nature of arrays in TypeScript, this avoids the issues mentioned:

declare const arr: Reactive<[string]>
declare let record: Record<number, string>
record = arr // ok

Any advice on these changes or a better approach?

@jh-leong
Copy link
Contributor Author

could you please rerun the ecosystem-ci

@Alfred-Skyblue
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented May 31, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools success success
nuxt success success
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify success success
vueuse success success
vue-simple-compiler success success

@yyx990803 yyx990803 merged commit aae2d78 into vuejs:main Jun 10, 2024
11 checks passed
@yyx990803
Copy link
Member

Great work!

yangmingshan added a commit to vue-mini/vue-mini that referenced this pull request Jun 15, 2024
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.

watch callback arg has incorrect type for reactive array containing refs
5 participants