Skip to content

set cssText: avoid serializing cssText when not needed#254

Merged
domenic merged 1 commit intojsdom:mainfrom
jsnajdr:fix/original-css-text
Dec 8, 2025
Merged

set cssText: avoid serializing cssText when not needed#254
domenic merged 1 commit intojsdom:mainfrom
jsnajdr:fix/original-css-text

Conversation

@jsnajdr
Copy link
Copy Markdown
Contributor

@jsnajdr jsnajdr commented Dec 1, 2025

Fixes jsdom/jsdom#3985, at least a big part of it.

During set cssText the library calls setProperty many times but wants to fire the onChange callback only once at the end. That's what the this._setInProgress field is for. But despite _setInProgress being true, the get cssText getter is called many times: to get the originalText, and because of suboptimal order of conditions.

This patch helps avoid these unneeded and expensive get cssText calls. They are expensive because they serialize the AST structure to a string.

I've been testing this on a little JSDOM benchmark script:

console.time('create-100-divs');
for (let i = 0; i < 100; i++) {
  const div = document.createElement('div');
  div.style.cssText = 'color: blue; background-color: white; padding: 10px; border: 1px solid black;';
  div.textContent = `Div #${i + 1}`;
  document.body.appendChild(div);
}
console.timeEnd('create-100-divs');

Before this PR, it takes 230ms to execute, after this PR it's 130ms, almost 2x improvement.

I think there are going to be even more optimization opportunities. If I remove the div.style.cssText assignment, the whole loop runs in 1ms.

@asamuzaK
Copy link
Copy Markdown
Contributor

asamuzaK commented Dec 1, 2025

I think this fix is already applied in another PR:
https://github.com/jsdom/cssstyle/pull/244/files#diff-c648a99dd91040422ecfb71b6bdaf3f98dee66a7860de9b015a09653f4761ea2R318

const originalText =
  typeof this._onChange === "function" && !this._updating ? this.cssText : null;

@jsnajdr
Copy link
Copy Markdown
Contributor Author

jsnajdr commented Dec 1, 2025

I think this fix is already applied in another PR:

Yes, I can confirm that both changes are applied there in the rewritten code.

Copy link
Copy Markdown
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Let me approve this and do a release, while we continue working on the larger refactoring.

@domenic domenic merged commit 73ce075 into jsdom:main Dec 8, 2025
3 checks passed
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.

Performance regression in jsdom@27

3 participants