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(renderer): fix patch error in unstable slot #9202

Closed
wants to merge 2 commits into from

Conversation

linghaoSu
Copy link
Contributor

@linghaoSu linghaoSu commented Sep 13, 2023

@github-actions
Copy link

github-actions bot commented Sep 13, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB (+37 B) 32.7 kB (+8 B) 29.5 kB (+1 B)
vue.global.prod.js 132 kB (+37 B) 49.4 kB (+8 B) 44.3 kB (-62 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB (+37 B) 18.9 kB (+8 B) 17.2 kB (+8 B)
createSSRApp 50.7 kB (+37 B) 20 kB (+9 B) 18.2 kB (+1 B)
defineCustomElement 50.3 kB (+37 B) 19.6 kB (+8 B) 17.9 kB (+14 B)
overall 61.3 kB (+37 B) 23.7 kB (+12 B) 21.6 kB (+31 B)

@baiwusanyu-c
Copy link
Member

please add unit test

n1.dynamicChildren
n1.dynamicChildren &&
// #9200 in some case stable fragment in deep unstable slot
n1?.children?.length === n2?.children?.length
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid using optional chaining #4882 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed optional chaining

Copy link
Member

@baiwusanyu-c baiwusanyu-c left a comment

Choose a reason for hiding this comment

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

avoid using optional chaining

n1.dynamicChildren
n1.dynamicChildren &&
// #9200 in some case stable fragment in deep unstable slot
n1?.children?.length === n2?.children?.length
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
n1?.children?.length === n2?.children?.length
dynamicChildren.length === n1.dynamicChildren.length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@linghaoSu
Copy link
Contributor Author

please add unit test

added unit test

@@ -351,4 +355,121 @@ describe('renderer: fragment', () => {
render(renderFn(['two', 'one']), root)
expect(serializeInner(root)).toBe(`text<div>two</div>text<div>one</div>`)
})

// #9200
test('stable fragment in unstable slot', () => {
Copy link
Member

Choose a reason for hiding this comment

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

this test case is too complex to understand.
here is a simple one

@edison1105
Copy link
Member

@linghaoSu
I opened a new PR but used another solution (maybe not the right fix).
so just keep this PR open.

@linghaoSu linghaoSu closed this Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

A comment in a template may cause an error under certain conditions
4 participants