-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: use wholeText
for only contenteditable
for set_data
#8394
Conversation
@baseballyama is attempting to deploy a commit to the Svelte Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! I'm wondering if we should split this up into two separate methods instead of having one do both
Oh yeah! Then we can reduce runtime level if statement. I will update it. |
); | ||
|
||
if (this.has_dynamic_attribute) { | ||
block.chunks.update.push(b` | ||
${fn}(${this.var}, ${data} = @get_spread_update(${levels}, [ | ||
block.chunks.update.unshift(b` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to switch to unshift
here and above? I switched it back to push
and the tests all passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, test case component-event-handler-contenteditable3
, get_spread_update
is added at ElementWrapper
class, and set_data_maybe_contenteditable
is added at MustacheTag
class.
And MustacheTag
is executed earlier than ElementWrapper
.
If get_spread_update
runs after set_data_maybe_contenteditable
, set_data_maybe_contenteditable
runs using div_data
that is before updating.
To avoid this situation, I use unshift
.
I think there are other ways to solve this issue, but I think this is reasonable and anyway, we will dynamically change the compiler in Svelte4, so we don't need to be particular about this.
set_attributes$(div$, div_data$ = get_spread_update$(div$_levels$, [dirty & /*spread*/ 2 && /*spread*/ ctx[1]]));
if (dirty & /*text*/ 1) set_data_maybe_contenteditable$(t$, /*text*/ ctx[0], div_data$['contenteditable']);
I checked the latest code. if (dirty & /*text*/ 1) set_data_maybe_contenteditable$(t$, /*text*/ ctx[0], "true"); |
This is just a memo but |
- remove unnecessary test - adjust one test to actually test a regression and skip it because it fails
In general I think comparing text nodes inside |
fix: #5931
close: #5940
I implemented compiler level check instead of runtime check.
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests
npm test
and lint the project withnpm run lint