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

only check is connected for dom nodes #4409

Merged
merged 3 commits into from
Jun 28, 2024
Merged

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Jun 15, 2024

isConnected is not present on text nodes, this prevents us from crashing due to calling getDomSibling for a text node. This does highlight that Google Translate leads to a bug, in the example given in #4318 (comment) we get the following as output

This is translated to Dutch, as you can see the h1 and the first hello reconcile correctly, however everything after the interpolated {label} does not.

Screenshot 2024-06-15 at 13 05 59

When I replace {props.label} with a normal string or with <span>{props.label}</span> it does work correctly.

This does however not solve the other issue mentioned on this PR, which does not have a way to reproduce. I reckon that one could be related to the dom-sibling of oldVNode also not being connected due to removing a lot of fragment-like wrapped children and oldDom being captured to the first of said list.

Copy link

github-actions bot commented Jun 15, 2024

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -1% - +0% (-11.43ms - +2.83ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -3% - +2% (-0.57ms - +0.33ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -5% - +3% (-3.31ms - +1.99ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -2% - +2% (-0.33ms - +0.31ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -3% - +1% (-2.43ms - +1.15ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -5% - +3% (-0.10ms - +0.06ms)
    preact-local vs preact-main
  • todo: faster ✔ 4% - 7% (1.11ms - 1.92ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -4% - +2% (-1.43ms - +0.65ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +2% (-0.30ms - +0.30ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -0% - +1% (-0.01ms - +0.04ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -0% - -0% (-0.00ms - -0.00ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -0% - -0% (-0.00ms - -0.00ms)
    preact-local vs preact-main

Results

create10k

duration

VersionAvg timevs preact-localvs preact-main
preact-local866.04ms - 872.89ms-unsure 🔍
-1% - +0%
-11.43ms - +2.83ms
preact-main867.51ms - 880.02msunsure 🔍
-0% - +1%
-2.83ms - +11.43ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local25.15ms - 25.15ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main25.15ms - 25.15msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms
-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.65ms - 17.07ms-unsure 🔍
-3% - +2%
-0.57ms - +0.33ms
preact-main16.58ms - 17.38msunsure 🔍
-2% - +3%
-0.33ms - +0.57ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.69ms - 1.69ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main1.69ms - 1.69msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms
-
hydrate1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local68.84ms - 72.11ms-unsure 🔍
-5% - +3%
-3.31ms - +1.99ms
preact-main69.04ms - 73.22msunsure 🔍
-3% - +5%
-1.99ms - +3.31ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local13.13ms - 13.59ms-unsure 🔍
-2% - +2%
-0.30ms - +0.30ms
preact-main13.17ms - 13.54msunsure 🔍
-2% - +2%
-0.30ms - +0.30ms
-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local18.18ms - 18.71ms-unsure 🔍
-2% - +2%
-0.33ms - +0.31ms
preact-main18.28ms - 18.63msunsure 🔍
-2% - +2%
-0.31ms - +0.33ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local4.61ms - 4.61ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main4.61ms - 4.61msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms
-
replace1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local81.45ms - 84.07ms-unsure 🔍
-3% - +1%
-2.43ms - +1.15ms
preact-main82.18ms - 84.62msunsure 🔍
-1% - +3%
-1.15ms - +2.43ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.54ms - 3.57ms-unsure 🔍
-0% - +1%
-0.01ms - +0.04ms
preact-main3.52ms - 3.56msunsure 🔍
-1% - +0%
-0.04ms - +0.01ms
-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local29.89ms - 30.45ms-unsure 🔍
-1% - +1%
-0.42ms - +0.42ms
preact-main29.86ms - 30.49msunsure 🔍
-1% - +1%
-0.42ms - +0.42ms
-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local35.42ms - 37.48ms-unsure 🔍
-3% - +4%
-1.26ms - +1.52ms
preact-main35.40ms - 37.26msunsure 🔍
-4% - +3%
-1.52ms - +1.26ms
-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local27.11ms - 27.59ms-unsure 🔍
-1% - +2%
-0.26ms - +0.43ms
preact-main27.01ms - 27.51msunsure 🔍
-2% - +1%
-0.43ms - +0.26ms
-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local33.72ms - 35.66ms-unsure 🔍
-3% - +4%
-1.06ms - +1.45ms
preact-main33.70ms - 35.29msunsure 🔍
-4% - +3%
-1.45ms - +1.06ms
-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local22.60ms - 24.33ms-unsure 🔍
-1% - +8%
-0.30ms - +1.81ms
preact-main22.10ms - 23.32msunsure 🔍
-8% - +1%
-1.81ms - +0.30ms
-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local25.30ms - 26.55ms-unsure 🔍
-2% - +5%
-0.51ms - +1.30ms
preact-main24.87ms - 26.18msunsure 🔍
-5% - +2%
-1.30ms - +0.51ms
-
text-update

duration

VersionAvg timevs preact-localvs preact-main
preact-local1.95ms - 2.06ms-unsure 🔍
-5% - +3%
-0.10ms - +0.06ms
preact-main1.96ms - 2.08msunsure 🔍
-3% - +5%
-0.06ms - +0.10ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local0.98ms - 0.98ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main0.98ms - 0.98msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms
-
todo

duration

VersionAvg timevs preact-localvs preact-main
preact-local25.59ms - 25.75ms-faster ✔
4% - 7%
1.11ms - 1.92ms
preact-main26.79ms - 27.58msslower ❌
4% - 7%
1.11ms - 1.92ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.25ms - 1.25ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
preact-main1.25ms - 1.25msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-
update10th1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local35.18ms - 36.07ms-unsure 🔍
-4% - +2%
-1.43ms - +0.65ms
preact-main35.08ms - 36.96msunsure 🔍
-2% - +4%
-0.65ms - +1.43ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.55ms - 3.55ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms
preact-main3.55ms - 3.55msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms
-

tachometer-reporter-action v2 for Benchmarks

Copy link

github-actions bot commented Jun 15, 2024

Size Change: +39 B (+0.06%)

Total Size: 61.8 kB

Filename Size Change
dist/preact.js 4.68 kB +7 B (+0.15%)
dist/preact.min.js 4.71 kB +6 B (+0.13%)
dist/preact.min.module.js 4.71 kB +6 B (+0.13%)
dist/preact.min.umd.js 4.74 kB +6 B (+0.13%)
dist/preact.module.js 4.7 kB +9 B (+0.19%)
dist/preact.umd.js 4.75 kB +5 B (+0.11%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.1 kB
compat/dist/compat.module.js 4.03 kB
compat/dist/compat.umd.js 4.17 kB
debug/dist/debug.js 3.7 kB
debug/dist/debug.module.js 3.71 kB
debug/dist/debug.umd.js 3.78 kB
devtools/dist/devtools.js 259 B
devtools/dist/devtools.module.js 273 B
devtools/dist/devtools.umd.js 345 B
hooks/dist/hooks.js 1.53 kB
hooks/dist/hooks.module.js 1.56 kB
hooks/dist/hooks.umd.js 1.6 kB
jsx-runtime/dist/jsxRuntime.js 981 B
jsx-runtime/dist/jsxRuntime.module.js 956 B
jsx-runtime/dist/jsxRuntime.umd.js 1.06 kB
test-utils/dist/testUtils.js 451 B
test-utils/dist/testUtils.module.js 456 B
test-utils/dist/testUtils.umd.js 536 B

compressed-size-action

@JoviDeCroock JoviDeCroock force-pushed the only-check-for-dom-nodes branch from 87376bc to 0a17073 Compare June 15, 2024 11:08
@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 4d8357a on only-check-for-dom-nodes
into 4c20c23 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 4d8357a on only-check-for-dom-nodes
into 4c20c23 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 0a17073 on only-check-for-dom-nodes
into 4c20c23 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 0a17073 on only-check-for-dom-nodes
into 4c20c23 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 0a17073 on only-check-for-dom-nodes
into 4c20c23 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 0a17073 on only-check-for-dom-nodes
into 4c20c23 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 0c05391 on only-check-for-dom-nodes
into 92e86e9 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 0c05391 on only-check-for-dom-nodes
into 92e86e9 on main.

@JoviDeCroock JoviDeCroock force-pushed the only-check-for-dom-nodes branch from 0c05391 to 4c114f9 Compare June 25, 2024 15:30
@JoviDeCroock JoviDeCroock force-pushed the only-check-for-dom-nodes branch from 4c114f9 to bd29648 Compare June 25, 2024 15:31
@coveralls
Copy link

Coverage Status

coverage: 99.482% (-0.1%) from 99.611%
when pulling bd29648 on only-check-for-dom-nodes
into eb37677 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.482% (-0.1%) from 99.611%
when pulling bd29648 on only-check-for-dom-nodes
into eb37677 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling bd29648 on only-check-for-dom-nodes
into eb37677 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling bd29648 on only-check-for-dom-nodes
into eb37677 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 75f1b71 on only-check-for-dom-nodes
into 76f5d66 on main.

@coveralls
Copy link

Coverage Status

coverage: 99.611%. remained the same
when pulling 75f1b71 on only-check-for-dom-nodes
into 76f5d66 on main.

@JoviDeCroock JoviDeCroock merged commit 16aeb9f into main Jun 28, 2024
13 checks passed
@JoviDeCroock JoviDeCroock deleted the only-check-for-dom-nodes branch June 28, 2024 12:56
@JoviDeCroock JoviDeCroock mentioned this pull request Jun 30, 2024
miguelrk added a commit to netzo/dsa that referenced this pull request Jul 9, 2024
…to DOM issues

See the following for the changes introduced which might have caused this:
- preactjs/preact#4409
- preactjs/preact#4421
- preactjs/preact#4403
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.

3 participants