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.

Sign up for GitHub

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

Jump to bottom

feat: Support MathML namespace #4364

Merged
merged 13 commits into from
May 2, 2024
Merged

feat: Support MathML namespace #4364

merged 13 commits into from
May 2, 2024

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Apr 30, 2024 •
edited
Loading

Closes #4363

Probably needs a proper refactor of isSvg to a namespace enum, but I need to play around with this a bit to see what we can skip & what we need. Done

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 30, 2024 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -1% - +1% (-8.99ms - +10.86ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +0% (-0.08ms - +0.07ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -3% - +3% (-2.47ms - +1.97ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -1% - +5% (-0.14ms - +0.74ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -2% - +3% (-1.18ms - +1.85ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -3% - +4% (-0.06ms - +0.07ms)
    preact-local vs preact-main
  • todo: slower ❌ 1% - 3% (0.17ms - 0.76ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -1% - +3% (-0.22ms - +0.78ms)
    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 🔍 -0% - +5% (-0.06ms - +0.66ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -0% - +1% (-0.00ms - +0.03ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -4% - +5% (-0.05ms - +0.06ms)
    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-local911.99ms - 917.20ms-unsure 🔍
-1% - +1%
-8.99ms - +10.86ms
preact-main904.08ms - 923.23msunsure 🔍
-1% - +1%
-10.86ms - +8.99ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local26.75ms - 26.75ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main26.75ms - 26.75msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.59ms - 16.69ms-unsure 🔍
-0% - +0%
-0.08ms - +0.07ms
preact-main16.59ms - 16.70msunsure 🔍
-0% - +0%
-0.07ms - +0.08ms		-

usedJSHeapSize

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

duration

VersionAvg timevs preact-localvs preact-main
preact-local76.29ms - 79.28ms-unsure 🔍
-3% - +3%
-2.47ms - +1.97ms
preact-main76.40ms - 79.68msunsure 🔍
-3% - +3%
-1.97ms - +2.47ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local14.14ms - 14.76ms-unsure 🔍
-0% - +5%
-0.06ms - +0.66ms
preact-main13.97ms - 14.32msunsure 🔍
-5% - +0%
-0.66ms - +0.06ms		-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.46ms - 17.09ms-unsure 🔍
-1% - +5%
-0.14ms - +0.74ms
preact-main16.17ms - 16.78msunsure 🔍
-4% - +1%
-0.74ms - +0.14ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local4.86ms - 4.86ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main4.85ms - 4.86msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-
replace1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local70.03ms - 71.46ms-unsure 🔍
-2% - +3%
-1.18ms - +1.85ms
preact-main69.07ms - 71.75msunsure 🔍
-3% - +2%
-1.85ms - +1.18ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.65ms - 3.68ms-unsure 🔍
-0% - +1%
-0.00ms - +0.03ms
preact-main3.64ms - 3.66msunsure 🔍
-1% - +0%
-0.03ms - +0.00ms		-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local28.45ms - 29.13ms-slower ❌
1% - 5%
0.38ms - 1.26ms
preact-main27.68ms - 28.26msfaster ✔
1% - 4%
0.38ms - 1.26ms		-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local35.79ms - 37.78ms-unsure 🔍
-1% - +7%
-0.19ms - +2.48ms
preact-main34.75ms - 36.53msunsure 🔍
-7% - +0%
-2.48ms - +0.19ms		-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local25.62ms - 25.96ms-slower ❌
1% - 3%
0.30ms - 0.79ms
preact-main25.07ms - 25.42msfaster ✔
1% - 3%
0.30ms - 0.79ms		-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local23.64ms - 24.40ms-slower ❌
1% - 6%
0.24ms - 1.31ms
preact-main22.87ms - 23.62msfaster ✔
1% - 5%
0.24ms - 1.31ms		-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local29.89ms - 31.66ms-unsure 🔍
-3% - +6%
-0.76ms - +1.69ms
preact-main29.46ms - 31.16msunsure 🔍
-5% - +2%
-1.69ms - +0.76ms		-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local23.91ms - 24.97ms-unsure 🔍
-1% - +6%
-0.19ms - +1.38ms
preact-main23.27ms - 24.42msunsure 🔍
-6% - +1%
-1.38ms - +0.19ms		-
text-update

duration

VersionAvg timevs preact-localvs preact-main
preact-local1.72ms - 1.81ms-unsure 🔍
-3% - +4%
-0.06ms - +0.07ms
preact-main1.72ms - 1.81msunsure 🔍
-4% - +3%
-0.07ms - +0.06ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.17ms - 1.25ms-unsure 🔍
-4% - +5%
-0.05ms - +0.06ms
preact-main1.17ms - 1.25msunsure 🔍
-5% - +4%
-0.06ms - +0.05ms		-
todo

duration

VersionAvg timevs preact-localvs preact-main
preact-local27.86ms - 28.14ms-slower ❌
1% - 3%
0.17ms - 0.76ms
preact-main27.28ms - 27.79msfaster ✔
1% - 3%
0.17ms - 0.76ms		-

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-local30.16ms - 31.06ms-unsure 🔍
-1% - +3%
-0.22ms - +0.78ms
preact-main30.10ms - 30.55msunsure 🔍
-3% - +1%
-0.78ms - +0.22ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.70ms - 3.70ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main3.69ms - 3.69msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-

tachometer-reporter-action v2 for Benchmarks

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 30, 2024 •
edited
Loading

Size Change: +154 B (0%)

Total Size: 61.2 kB

Filename Size Change
dist/preact.js 4.63 kB +24 B (+1%)
dist/preact.min.js 4.67 kB +24 B (+1%)
dist/preact.min.module.js 4.66 kB +20 B (0%)
dist/preact.min.umd.js 4.69 kB +26 B (+1%)
dist/preact.module.js 4.65 kB +32 B (+1%)
dist/preact.umd.js 4.71 kB +28 B (+1%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.09 kB
compat/dist/compat.module.js 4.01 kB
compat/dist/compat.umd.js 4.14 kB
debug/dist/debug.js 3.63 kB
debug/dist/debug.module.js 3.62 kB
debug/dist/debug.umd.js 3.71 kB
devtools/dist/devtools.js 231 B
devtools/dist/devtools.module.js 240 B
devtools/dist/devtools.umd.js 314 B
hooks/dist/hooks.js 1.55 kB
hooks/dist/hooks.module.js 1.59 kB
hooks/dist/hooks.umd.js 1.63 kB
jsx-runtime/dist/jsxRuntime.js 976 B
jsx-runtime/dist/jsxRuntime.module.js 949 B
jsx-runtime/dist/jsxRuntime.umd.js 1.06 kB
test-utils/dist/testUtils.js 453 B
test-utils/dist/testUtils.module.js 454 B
test-utils/dist/testUtils.umd.js 536 B

compressed-size-action

@coveralls
Copy link

coveralls commented Apr 30, 2024 •
edited
Loading

Coverage Status

coverage: 99.609%. remained the same
when pulling 12b71cf on feat/mathml
into f7e9bcb on main.

rschristian
rschristian commented Apr 30, 2024
View reviewed changes
src/component.js Outdated Show resolved Hide resolved
@rschristian rschristian force-pushed the feat/mathml branch 2 times, most recently from 7200207 to 06a0d73 Compare April 30, 2024 09:18
@rschristian rschristian force-pushed the feat/mathml branch 2 times, most recently from 4c7e8f6 to 7f3d7cb Compare April 30, 2024 13:01
@rschristian rschristian marked this pull request as ready for review April 30, 2024 13:06
@rschristian rschristian changed the title feat: Support creating MathML elements feat: Support MathML namespace Apr 30, 2024
lukewarlow
lukewarlow reviewed Apr 30, 2024
View reviewed changes
Copy link

@lukewarlow lukewarlow left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with Preact internals but this change looks good overall to me. It touches the various places I found when reporting the issue.

Perhaps some tests that handle conditional rendering would ensure context isn't lost in those cases?

@rschristian
Copy link
Member Author

Yeah that's fair -- will try to add later (for SVGs too)

JoviDeCroock
JoviDeCroock approved these changes May 1, 2024
View reviewed changes
Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for tackling this! I left two size-nits

src/component.js Outdated Show resolved Hide resolved
src/render.js Outdated Show resolved Hide resolved
rschristian
rschristian commented May 1, 2024
View reviewed changes
src/render.js Outdated Show resolved Hide resolved
@rschristian rschristian force-pushed the feat/mathml branch 3 times, most recently from e985a80 to 2d07ca8 Compare May 1, 2024 22:37
rschristian
rschristian commented May 1, 2024
View reviewed changes
src/diff/index.js
Comment on lines +379 to +382
if (nodeType === 'svg') namespace = 'http://www.w3.org/2000/svg';
else if (nodeType === 'math')
namespace = 'http://www.w3.org/1998/Math/MathML';
else if (!namespace) namespace = 'http://www.w3.org/1999/xhtml';
Copy link
Member Author

Choose a reason for hiding this comment

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

Switching this to a ternary shaves off -6b or so but performs a fair bit worse.

@rschristian rschristian merged commit 4ab4b99 into main May 2, 2024
13 checks passed
@rschristian rschristian deleted the feat/mathml branch May 2, 2024 03:19
@JoviDeCroock JoviDeCroock mentioned this pull request May 15, 2024
Merged
@X-oss-byte X-oss-byte mentioned this pull request Jun 5, 2024
Open
@Jircs1 Jircs1 mentioned this pull request Jun 10, 2024
Open
This was referenced Jun 15, 2024
Open
Open
Open
@vAHiD55555 vAHiD55555 mentioned this pull request Jun 20, 2024
Open
@X-oss-byte X-oss-byte mentioned this pull request Jul 22, 2024
Open
@lorenzoAtBEIS lorenzoAtBEIS mentioned this pull request Aug 10, 2024
Merged
@ZeroXClem ZeroXClem mentioned this pull request Aug 12, 2024
Closed
This was referenced Aug 14, 2024
Open
Open
@ramzimalhas ramzimalhas mentioned this pull request Aug 22, 2024
Open
This was referenced Sep 3, 2024
Open
Open
@arkid15r arkid15r mentioned this pull request Sep 11, 2024
Closed
@ntavassoli-dot ntavassoli-dot mentioned this pull request Sep 13, 2024
Open
This was referenced Sep 13, 2024
Open
Open
@organich organich mentioned this pull request Sep 24, 2024
Open
@X-oss-byte X-oss-byte mentioned this pull request Oct 5, 2024
Open
@X-oss-byte X-oss-byte mentioned this pull request Oct 18, 2024
Open
@X-oss-byte X-oss-byte mentioned this pull request Oct 25, 2024
Open
@Xcaciv Xcaciv mentioned this pull request Oct 27, 2024
Open
@dwhiteGUK dwhiteGUK mentioned this pull request Oct 28, 2024
Open
@abaum65 abaum65 mentioned this pull request Oct 31, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukewarlow lukewarlow lukewarlow left review comments

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect namespacing of MathML elements
4 participants
@rschristian @coveralls @JoviDeCroock @lukewarlow