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(reactivity): assigning array.length while observing a symbol property #7568

Merged
merged 2 commits into from
Oct 21, 2023

Conversation

skirtles-code
Copy link
Contributor

@skirtles-code skirtles-code commented Jan 23, 2023

Example 1: SFC Playground

<script setup>
import { reactive } from 'vue'

const s = Symbol()
const a = reactive([1])

setTimeout(() => {
  a.length = 0
}, 100)
</script>

<template>
  {{ a }} {{ a[s] }}
</template>

Chrome:

Uncaught TypeError: Cannot convert a Symbol value to a number

Firefox:

Uncaught TypeError: can't convert symbol to number

Assigning the length of a reactive array will trigger this error if we are also observing a symbol-keyed property on the same array. The logic for assigning the length iterates through all observed properties, but doesn't take into account the possibility that a key could be a symbol.

This can also be triggered by pop(), shift() and splice(), which set the length internally.

I originally encountered this problem in a question on Vue Land, where somebody was using the stringify function from yaml. That function checks for a special symbol as part of the conversion process. Just checking for the symbol is enough to add a dependency, it doesn't matter whether the property exists:

Example 2: SFC Playground


The same examples using the deploy preview of this PR:

@yingpengsha
Copy link

I need this change, but no one seems to care except us.

@nawbc
Copy link

nawbc commented Feb 17, 2023

@yyx990803 cc

1 similar comment
@yisar
Copy link

yisar commented Feb 17, 2023

@yyx990803 cc

@yingpengsha
Copy link

Hi, Evan @yyx990803
The reason I need this change is that I am trying to apply @vue/reactivity to solid-js using the enableExternalSource of solid-js. At the same time, I think this change will not bring too many side effects. I hope this change can be approved.

@yingpengsha
Copy link

@sxzz PTAL

@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB (+7 B) 32.7 kB (+3 B) 29.5 kB (+16 B)
vue.global.prod.js 132 kB (+7 B) 49.4 kB (+2 B) 44.3 kB (-30 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB (+7 B) 18.9 kB (+6 B) 17.2 kB (+33 B)
createSSRApp 50.7 kB (+7 B) 20 kB (+5 B) 18.2 kB (-2 B)
defineCustomElement 50.3 kB (+7 B) 19.6 kB (+3 B) 17.9 kB (-24 B)
overall 61.2 kB (+7 B) 23.7 kB (+2 B) 21.5 kB (-6 B)

@yyx990803 yyx990803 merged commit e9e2778 into vuejs:main Oct 21, 2023
11 checks passed
lumozx pushed a commit to lumozx/core that referenced this pull request Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants