Skip to content

fix: improve code quality with type annotations and error handling #14570

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.

Sign up for GitHub

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

Open
N3XT3R1337 wants to merge 1 commit into
base: main
Choose a base branch
from
+42 −2
Open

fix: improve code quality with type annotations and error handling #14570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/runtime-core/__tests__/apiWatch.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1446,6 +1446,33 @@ describe('api: watch', () => {
expect(spy).toHaveBeenCalledTimes(2)
})

test('watching keypath should not stop on falsy intermediate values', async () => {
const spy = vi.fn()
const Comp = defineComponent({
render() {},
data() {
return {
a: {
b: 0 as any,
},
}
},
created(this: any) {
this.$watch('a.b', spy)
},
mounted(this: any) {
this.a.b = 1
},
})

const root = nodeOps.createElement('div')
createApp(Comp).mount(root)

await nextTick()
expect(spy).toHaveBeenCalledTimes(1)
expect(spy).toHaveBeenCalledWith(1, 0, expect.anything())
})
Comment on lines +1449 to +1474
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Test case does not exercise a falsy intermediate segment.

On Line 1449, a.b makes 0 the terminal value. This can pass even without the cur != null fix, so the regression target is under-tested.

✅ Suggested test addition to cover a true intermediate-falsy traversal 
+  test('watching keypath should continue through falsy intermediate values', async () => {
+    const spy = vi.fn()
+    const Comp = defineComponent({
+      render() {},
+      data() {
+        return {
+          a: {
+            b: '' as any,
+          },
+        }
+      },
+      created(this: any) {
+        this.$watch('a.b.value', spy)
+      },
+      mounted(this: any) {
+        this.a.b = { value: 1 }
+      },
+    })
+
+    const root = nodeOps.createElement('div')
+    createApp(Comp).mount(root)
+
+    await nextTick()
+    expect(spy).toHaveBeenCalledTimes(1)
+    expect(spy).toHaveBeenCalledWith(1, undefined, expect.anything())
+  })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test('watching keypath should not stop on falsy intermediate values', async () => {
const spy = vi.fn()
const Comp = defineComponent({
render() {},
data() {
return {
a: {
b: 0 as any,
},
}
},
created(this: any) {
this.$watch('a.b', spy)
},
mounted(this: any) {
this.a.b = 1
},
})
const root = nodeOps.createElement('div')
createApp(Comp).mount(root)
await nextTick()
expect(spy).toHaveBeenCalledTimes(1)
expect(spy).toHaveBeenCalledWith(1, 0, expect.anything())
})
test('watching keypath should not stop on falsy intermediate values', async () => {
const spy = vi.fn()
const Comp = defineComponent({
render() {},
data() {
return {
a: {
b: 0 as any,
},
}
},
created(this: any) {
this.$watch('a.b', spy)
},
mounted(this: any) {
this.a.b = 1
},
})
const root = nodeOps.createElement('div')
createApp(Comp).mount(root)
await nextTick()
expect(spy).toHaveBeenCalledTimes(1)
expect(spy).toHaveBeenCalledWith(1, 0, expect.anything())
})
test('watching keypath should continue through falsy intermediate values', async () => {
const spy = vi.fn()
const Comp = defineComponent({
render() {},
data() {
return {
a: {
b: '' as any,
},
}
},
created(this: any) {
this.$watch('a.b.value', spy)
},
mounted(this: any) {
this.a.b = { value: 1 }
},
})
const root = nodeOps.createElement('div')
createApp(Comp).mount(root)
await nextTick()
expect(spy).toHaveBeenCalledTimes(1)
expect(spy).toHaveBeenCalledWith(1, undefined, expect.anything())
})
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/runtime-core/__tests__/apiWatch.spec.ts` around lines 1449 - 1474,
The test currently watches 'a.b' so the falsy value 0 is the terminal value;
change the watched path to include an extra segment (e.g. 'a.b.c') so the falsy
0 is an intermediate segment: update the definition in the test (Comp.data) to
keep a.b = 0 as the intermediate falsy, change the this.$watch call in created
to this.$watch('a.b.c', spy), and in mounted assign this.a.b = { c: 1 } (or set
this.a.b.c = 1 after replacing b with an object) so the watcher exercises
traversal over a falsy intermediate value and the assertions expect the new
value 1 and previous undefined.


it('watching sources: ref<any[]>', async () => {
const foo = ref([1])
const spy = vi.fn()
Expand Down
2 changes: 1 addition & 1 deletion packages/runtime-core/src/apiWatch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ export function createPathGetter(
const segments = path.split('.')
return (): WatchSource | WatchSource[] | WatchEffect | object => {
let cur = ctx
for (let i = 0; i < segments.length && cur; i++) {
for (let i = 0; i < segments.length && cur != null; i++) {
cur = cur[segments[i] as keyof typeof cur]
}
return cur
Expand Down
11 changes: 11 additions & 0 deletions packages/runtime-dom/__tests__/patchStyle.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,15 @@ describe(`runtime-dom: style patching`, () => {
patchProp(el, 'style', 'color:red', { fontSize: '12px' })
expect(el.style.cssText.replace(/\s/g, '')).toBe('font-size:12px;')
})

it('should handle string style with trailing semicolon when patching to object', () => {
const el = document.createElement('div')
// set initial string style with trailing semicolon
patchProp(el, 'style', {}, 'color:red;')
expect(el.style.cssText.replace(/\s/g, '')).toBe('color:red;')

// patch from string to object, entries without colon (from trailing ;) should be skipped
patchProp(el, 'style', 'color:red;', { fontSize: '12px' })
expect(el.style.cssText.replace(/\s/g, '')).toBe('font-size:12px;')
})
})
4 changes: 3 additions & 1 deletion packages/runtime-dom/src/modules/style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ export function patchStyle(el: Element, prev: Style, next: Style): void {
}
} else {
for (const prevStyle of prev.split(';')) {
const key = prevStyle.slice(0, prevStyle.indexOf(':')).trim()
const colonIndex = prevStyle.indexOf(':')
if (colonIndex < 0) continue
const key = prevStyle.slice(0, colonIndex).trim()
if (next[key] == null) {
setStyle(style, key, '')
}
Expand Down