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

Enhance patch props in element by hasPropsChanged #5773

Closed
masterX89 opened this issue Apr 21, 2022 · 3 comments
Closed

Enhance patch props in element by hasPropsChanged #5773

masterX89 opened this issue Apr 21, 2022 · 3 comments

Comments

@masterX89
Copy link

masterX89 commented Apr 21, 2022

What problem does this feature solve?

Consider a test case like this:

it('should not update element props which is already mounted when props are same', () => {
  render(h('div', { id: 'bar' }, ['foo']), root)
  expect(inner(root)).toBe('<div id="bar">foo</div>')

  render(h('div', { id: 'bar' }, ['foo']), root)
  expect(inner(root)).toBe('<div id="bar">foo</div>')
})

oldProps and newProps are actually the same when updating elemtent props, but since they are objects, oldProps !== newProps still return true like this line in renderer

// patchProps
if (oldProps !== newProps) {
    for (const key in newProps) {/* handle newProps */}
    if (oldProps !== EMPTY_OBJ) {/* handle oldProps */}
    if ('value' in newProps) {/* handle 'value' prop */}
}

Is it possible to consider using hasPropsChanged in componentRenderUtils instead of oldProps !== newProps to improve performance.
Please confirm if the scheme is feasible, I tried modifying to hasPropsChanged and all tests passed, ready to PR.

What does the proposed API look like?

no proposed API

@LinusBorg
Copy link
Member

LinusBorg commented Apr 21, 2022

In the body of for (const key in newProps) {/* handle newProps */}, which you commented out, we do check wether the old value is equal to the new one before doing an actual patch, so we kinds already do what hasPropsChanged does. though yes, we do a second loop over oldProps after this first one.

Your proposed change would effectively save one loop over the props keys when props did not change, and add one more loop over the props keys when props did change.

This seems like a small net benefit in (the likely common) scenarios where you have more elements with unchanged props than elements with changed props in a given re-render.

@masterX89
Copy link
Author

Your proposed change would effectively save one loop over the props keys when props did not change, and add one more loop over the props keys when props did change.

This seems like a small net benefit in (the likely common) scenarios where you have more elements with unchanged props than elements with changed props in a given re-render.

I agree with you, it seems to be a balance issue at the design level, one scenario will save one loop, and one scenario will add one loop.
From the design perspective of the vue.js framework, do I need to modify this part of the code?

@masterX89
Copy link
Author

see #5857

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants