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: prevent unnecessary property updating in set_attributes #6809

Closed
wants to merge 6 commits into from
Closed

fix: prevent unnecessary property updating in set_attributes #6809

wants to merge 6 commits into from

Conversation

baseballyama
Copy link
Member

@baseballyama baseballyama commented Oct 4, 2021

fix: #6751, #6752

This is an alternative way of #6758.

Original PR (#6758) uses dependencies.size before call get_dependencies().
But in my understanding, dependencies have an attribute's dependencies.
But get_dependencies() returns parent node's dependencies.
So below logic looks a little bit weird to me.
(But honestly, I couldn't find any examples of anything being broken by this fix. So maybe this fix is okay...)
(If my understanding is wrong, please correct me.)

const condition = attr.node.dependencies.size > 0
  ? block.renderer.dirty(attr.node.get_dependencies())
  : null;

So I think the original code is okay regarding this part.


I think the root cause is different behavior between normal variables and spread syntax.
But I don't know how to fix it.

But I think, this behavior becomes an issue only during user input.
And I think in most cases, attributes are not changed during user input.

Therefore I thought 2 ways to fix this issue.

1. Stop calling unnecessary set_attributes.

The runtime calls set_attributes even if min is not changed but it's not necessary.
So I thought if we can handle it at compile process, this is good.
But associative array (or JSON) can have nested objects.
Therefore compile process will be significantly complicated or impossible to handle.
And I think this is too much at least for this issue.
So I didn't choose this way.

2. Stop setting value if it's not changed (I choose this way in the PR)

This is a simple way.
Based on the assumption that the attributes will not change during user input,
set_attributes will not apply new value if it's not changed.
Actually already attr function did this way.
So It'll just be the same process.


Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

I'm so sorry If @Wojmis3 felt impolite,
And I apologize for the length of the text.

Thank you for reading!🎉

@baseballyama
Copy link
Member Author

fix: #6752

I think this issue is also much similar to #6751.
So I added 1 more commit to this PR.

But still, I don't get a comment from maintainer regarding this issue and PR.
So I'm not sure this fix is reasonable.
But personally, I think this is one of reasonable way.
Therefore I added this commit.

Thank you!

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Is there any way we can test this?

src/runtime/internal/dom.ts Outdated Show resolved Hide resolved
@vercel
Copy link

vercel bot commented Feb 23, 2023

@baseballyama is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@baseballyama
Copy link
Member Author

@dummdidumm

I tried to add a test but I can not make focus on date part of date input.
Therefore I can not test for this.
So in terms of adding a test, I added a comment.

スクリーンショット 2023-02-23 13 47 03

@benmccann benmccann changed the title [fix] Prevent unnecessary property updating in set_attributes fix: prevent unnecessary property updating in set_attributes Mar 14, 2023
@dummdidumm
Copy link
Member

The Chrome date input issue is no longer reproducible, and this PR does not solve the bug with width not updating, therefore closing.

@dummdidumm dummdidumm closed this Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date input types weirdly when date boundaries are set with spread attributes in Chrome
3 participants