cssstyle Refactoring Project: Split PR Plan

[asamuzaK]

Planned by Gemini.
Thoughts?

cssstyle Refactoring Project: Split PR Plan

Objective: To decouple CSSStyleDeclaration and migrate property logic to lib/properties through a series of reviewable, incremental steps.

Overall Roadmap

Order	Category	PR Title Draft	Main Changes	Impact Scope
1	Foundation	Add shared parsers and property interface	Addition of shared parsers, creation of directory structure.	No impact on existing code.
2	Core	Refactor CSSStyleDeclaration core & registry	Overhaul of CSSStyleDeclaration.js, implementation of the registry.	Breaking Changes (Unmigrated properties will temporarily stop working).
3	Migration	Port basic properties (Color, Font, etc.)	Migration of basic properties, fixing E2E tests.	Restoration of basic functionality.
4	Migration	Port remaining properties (Batch 1-N)	Mechanical/Manual migration of remaining properties.	Restoration of full functionality.
5	Cleanup	Remove legacy generator scripts	Removal of old scripts/generateProperties.js, etc.	Cleanup.

Detailed PR Plans

PR 1: Foundation

Goal: Prepare the common functions required by the new property definition files.

• Changes:
  • Create lib/parsers/utils.js (or lib/parsers/*.js).
  • Implement utility functions for parsing colors (parseColor), lengths (parseLength), and processing keywords.
  • Create the lib/properties/ directory (initially empty or with a .gitkeep).
• Review Points:
  • Are the regular expressions and logic for the parsers correct?
  • Are unit tests (test/parsers.js, etc.) added?

PR 2: The Core Swap

Goal: Discard the previous tightly coupled CSSStyleDeclaration and replace it with the generic container version.

• Changes:
  • Rewrite lib/CSSStyleDeclaration.js to the new Map-based implementation.
  • Create lib/properties/index.js (the registry).
  • Important: Since most properties will be unregistered at this point, many existing tests may fail. Add only 1-2 sample properties (like color or display) to lib/properties/ to prove the core logic works minimally.
• Review Points:
  • Is the generic logic for setProperty / getPropertyValue correct?
  • Is dynamic loading from the registry functioning correctly?

PR 3: Core Properties Migration

Goal: Migrate frequently used major properties and establish the "pattern" for the project.

• Changes:
  • Create major properties like color, font-size, background, margin, padding in lib/properties/*.js.
  • Register them in lib/properties/index.js.
  • Ensure existing tests regarding these properties pass.
• Review Points:
  • Are initialValue and inherited settings correct?
  • Are the parsers created in PR 1 being used correctly?

PR 4: Bulk Migration

Goal: Migrate all remaining hundreds of properties to the new format.
Note: Due to the volume, split this into further PRs if necessary (e.g., PR 4-1: Layout props, PR 4-2: SVG props).

• Changes:
  • Split all remaining properties into individual files.
  • Register all of them in the registry.
• Review Points:
  • Are there any mechanical errors (copy-paste errors)?
  • Do all test cases (including WPT) pass?

PR 5: Cleanup

Goal: Remove unnecessary generation scripts and old definition files to clean up the project.

• Changes:
  • Remove scripts/generateProperties.js.
  • Remove the lib/generated/ directory.
  • Modify build scripts in package.json (remove the generation step).
• Review Points:
  • Is anything forgotten to be deleted?
  • Does a clean install (npm ci) followed by test execution work correctly?

Development & Merge Strategy Recommendation

This change will cause temporary dysfunction (many properties will disappear) at the stage of PR 2. Therefore, the following strategy is recommended:

1. Use a Feature Branch: Do not merge directly into the main branch. Instead, create a long-lived feature branch (e.g., refactor/decouple-properties) and merge PRs 1 through 5 into it.
2. Alternative (Parallel Existence - High Difficulty): It is possible to include a "fallback to new implementation" within the old implementation in PR 2 to allow coexistence, but this complicates the code. Thus, the feature branch strategy is recommended for this refactoring.

[domenic]

I'm supportive of this plan, using the feature-branch approach. However, I think it focuses a lot on breaking down the "core work" in #244, and does not focus on the things that made it especially hard for me to review.

In particular, I would strongly appreciate it if small changes got their own PR. Referring to #244 (comment), this includes things like:

• A lot of style changes to use constants instead of strings
• Updates to the CI stuff
• Updates to the generated properties
• The introduction of a caching system
• An optimization to do less serializing (set cssText: avoid serializing cssText when not needed #254)
• Updates to dependencies and dev dependencies
• Updates to the main module's exports to include property lists

(Also, I noticed some updates to use stronger jsdoc.)

All of those could be separate PRs. Having to review changes like that mixed together with the core logic changes is part of what makes reviews hard and take longer.

How do you feel about this feedback?

[asamuzaK]

cssstyle Refactoring Project: Revised Split PR Plan

Objective: To decouple CSSStyleDeclaration and migrate property logic to lib/properties through a series of reviewable, incremental steps, while incorporating performance optimizations and strictly managing scope.

Overall Roadmap

Order	Category	PR Title Draft	Main Changes	Impact Scope
1	Foundation	Add shared parsers, constants, and utils	parsers, constants, directory structure.	No impact on existing code.
2	Core	Refactor CSSStyleDeclaration core & registry	CSSStyleDeclaration rewrite, Registry.	Breaking Changes (Temporary dysfunction).
3	Migration	Port basic properties (Color, Font, etc.)	Migration & update of basic properties.	Restoration of basic functionality.
4	Migration	Port remaining properties (Batch 1-N)	Bulk migration of remaining properties.	Restoration of full functionality.
5	Cleanup	Remove legacy generator scripts	Removal of old scripts/folders.	Cleanup.
6	Optimization	Introduce caching & serialization improvements	Caching system, lazy serialization.	Performance improvement.
7	Finalization	Finalize Public API	Strictly define the main module's exports.	Final API stability.

Detailed PR Plans

PR 1: Foundation

Goal: Prepare the common functions and constants required by the new property definition files.

• Changes:
  • Use constants instead of strings: Define constants for frequently used strings (e.g., 'important', 'initial', 'inherit', parsing keywords) to prevent typos and improve maintainability.
  • Create lib/parsers/utils.js (or lib/parsers/*.js).
  • Implement utility functions for parsing colors (parseColor), lengths (parseLength).
  • Prepare the lib/properties/ directory. (Note: Existing files will be kept as-is for now and refactored in PR 3 & 4).
• Review Points:
  • Are magic strings replaced with constants where appropriate?
  • Are the regular expressions and logic for the parsers correct?

PR 2: The Core Swap

Goal: Discard the previous tightly coupled CSSStyleDeclaration and replace it with the generic container version.

• Changes:
  • Rewrite lib/CSSStyleDeclaration.js to the new Map-based implementation.
  • Create lib/properties/index.js (the registry).
  • Initial Verification: Add 1-2 sample properties to prove the core logic.
• Review Points:
  • Is the generic logic for setProperty / getPropertyValue correct?
  • Is dynamic loading from the registry functioning correctly?

PR 3: Core Properties Migration

Goal: Migrate frequently used major properties.

• Changes:
  • Updates to the generated properties: Manually port major properties (color, font, etc.) to lib/properties/*.js. During this process, verify and update their definitions (initial values, inheritance) to match the latest specs, rather than blindly copying legacy logic.
  • Register them in the registry.
• Review Points:
  • Do the ported properties behave correctly according to the new architecture?

PR 4: Bulk Migration

Goal: Migrate all remaining hundreds of properties.

• Changes:
  • Split all remaining properties into individual files.
  • Updates to the generated properties: Ensure all ported properties are updated to use the new constants and parsers defined in PR 1.
• Review Points:
  • Do all test cases (including WPT) pass?

PR 5: Cleanup

Goal: Remove unnecessary generation scripts and old definition files.

• Changes:
  • Remove scripts/generateProperties.js.
  • Remove the lib/generated/ directory.

PR 6: Optimization & Refinement

Goal: Improve performance after the architectural refactoring is complete.

• Changes:
  • Introduce caching system: Implement a caching mechanism (e.g., caching parsed values or computed styles) to reduce repetitive parsing overhead.
  • An optimization to do less serializing: Optimize cssText or internal serialization logic to compute strings only when necessary (lazy evaluation) or cache the serialized result until invalidation.
• Review Points:
  • Does the caching logic correctly invalidate when properties change?
  • Is there a measurable performance gain?

PR 7: Finalize Public API

Goal: Lock down the external interface of the package.

• Changes:

  • Updates to the main module's exports: Define the public API strictly as follows. This is the assumed final shape for this refactoring. Any need for additional exports will be addressed by revising the final plan at that time.

    'use strict';
    
    const CSSStyleDeclaration \= require('./CSSStyleDeclaration');  
    const { allProperties } \= require('./properties');
    
    module.exports \= {  
      CSSStyleDeclaration,  
      properties: allProperties  
    };

• Review Points:

  • Is the property list correctly exported as properties?
  • Does the exported object match the final design requirements?

Out of Scope

The following items are explicitly excluded from this refactoring plan to keep the focus on architecture and logic:

• Updates to the CI (workflow): No changes to GitHub Actions or test runners unless absolutely necessary for the build to pass.
• Updates to dependencies and dev dependencies: No upgrades of external packages (e.g., eslint, jest, etc.).

[domenic]

I think that plan is worse, unfortunately. I think you might need to apply some critical thinking here instead of relying on AI.

The principle for a good PR should be:

• Is this change useful in advancing toward the end state?
• Is this change easy to review?
• Does everything that changes in this PR have a single goal?

A lot of the proposals above violate that. For example, PR 1 bundles together using constants instead of strings with other things. PR 2 bundles together a new Map-based implementation with other things. On the other hand, leaving dead files around and then having a dedicated PR to clean them up is very confusing: files should be removed in the same PR that makes them obsolete.

I'd encourage you to think about this more deeply. Look at the diff in https://github.com/jsdom/cssstyle/pull/244/files. Which of those changes can be done with no dependencies on other parts of the PR? Then just start doing them. No need to plan so hard. I think the list I gave twice now (most recently in #255 (comment)) gives a pretty solid list. Why don't you just send a single PR to update GitHub actions? Then a single PR to update the dependencies and dev dependencies? Then a single PR to use constants instead of strings? Then a single PR to add more jsdoc? Then a single PR to make whatever other style changes you like? Etc. I feel like once you exhaust the list of easy separable PRs, it'll become a lot clearer what's left.

[asamuzaK]

It's unfortunate that there was a misunderstanding at the beginning between ourselves, but as I said before, I frankly have no intention of splitting the PR and resubmitting them from scratch MYSELF.
As a compromise, I was willing to help if AI (Gemini) to make a proposal and then make minor adjustments based on that proposal.

Have you tried jsdom/jsdom#3957 locally using the npm link, which applies #244?
All tests are green and all the issues mentioned in 3957 are fixed.

Due to an initial misunderstanding, the acemir/CSSOM update has been included in that PR, but it can be removed.
CSSRules-related npm tests should fail, but that can be addressed as a separate issue.

If it is not acceptable, feel free to close this issue and the PR 244.
Thanks.

[domenic]

I see. If you are not willing to accept feedback on the points mentioned in #244 (comment), then probably we should not proceed with that work. Feel free to close it.

Bug and regression fixes are always welcome, but submitting re-architecture PRs and not accepting guidance or being willing to break them into more understandable pieces is not generally a good fit for contributing to open source projects.

[asamuzaK]

That's something I don't quite understand.
#244 (comment)
You say, "That is, we remove from this package every public API that exposes individual properties from this package."
But it's probably impossible to remove properties/* from cssstyle.
This is because cssText, getPropertyValue(property), item(i), internal shorthand handlers and so on all depend in some way on the individual files in properties/*.
In other words, properties/* need to be kept in cssstyle and not in jsdom.

However, as for the additions made to CSSStyleDeclaration like:

Object.defineProperties(CSSStyleDeclaration.prototype, generatedProperties);

and

for (const definition of propertyDefinitions.values()) {
  (snip)
}

I think it's possible to export it as a separate object from CSSStyleDeclaration in terms of dividing up the responsibilities of declarations and properties.

[domenic]

You say, "That is, we remove from this package every public API that exposes individual properties from this package."
But it's probably impossible to remove properties/* from cssstyle.

I don't think these things are connected. That is, I was not trying to suggest removing properties/*. I was trying to suggest removing CSSStyleDeclaration.prototype.backgroundPosition, etc.

setProperty(), getPropertyValue(), cssText and so on would still delegate to the logic in properties/*.

I will try to spell it out in more detailed code:

class CSSStyleDeclaration {
  // not shown here, but included in the public API:
  // - get cssText, set cssText
  // - get length, item()
  // - getPropertyValue(), getPropertyPriority(), removeProperty()
  // - get parentRule

  setProperty(property, value, priority) {
    // most logic the same as today, but replacing this[property] = value with
    if (Object.hasOwn(generatedProperties, property)) {
      generatedProperties[property].set.call(this, value);
    } else {
      const parsedValue = parsers.parsePropertyValue(property, value, { globalObject: this._global });
      this._setProperty(property, parsedValue);
    }
  }
}

// There is no CSSStyleDeclaration.prototype.backgroundImage, etc.

// Ideally, there is no cssStyleDeclarationInstance[0], [1], etc. Only item(i). But that can be a separate PR if needed.

Then in jsdom the property stuff is simple, and matches the spec:

// CSSStyleDeclaration-impl.js
module.exports = require("cssstyle").CSSStyleDeclaration;

// CSSStyleProperties-impl.js
module.exports = class CSSStylePropertiesImpl extends CSSStyleDeclarationImpl { /* ... */ };

for (const [propertyOrAlias, property] of giantListOfPropertiesFromCssstylePackage) {
  Object.defineProperty(module.exports.prototype, propertyOrAlias, {
    get() {
      return this.getPropertyValue(property);
    },
    set(value) {
      this.setProperty(property, value);
    }
  });
}

The main goal (which I thought was the goal we agreed upon back in #235 (comment)) is to move away from individual properties on CSSStyleDeclaration.prototype for every CSS property. The logic instead lives in setProperty().

We'll still need all the properties/* files for some time. (Unless we can eventually simplify all that by using csstree or something.) But compare the following:

Current architecture:

• user calls el.style.setProperty() -> el.style[property] = value -> generated code in properties/* installed on the prototype -> el.style._setProperty()
• user calls el.style.backgroundPosition = value -> generated code in properties/* installed on the prototype -> el.style._setProperty()

As discussed in #235, this architecture has the following drawbacks:

• It doesn't match the spec, which splits up CSSStyleDeclaration and CSSStyleProperties, and has setters/getters like backgroundPosition defined in terms of setProperty()/getPropertyValue().
• It doesn't have a webidl2js layer, so it has to take care of a lot of annoying stuff that webidl2js already handles, such as indexed property support, hiding the internals from the user, validating arguments.
• It mixes together the concerns of defining parser and serializer behavior for each property, with defining property descriptors and aliases for each property.

Your proposal using the current version of #244 + jsdom/jsdom#3957:

• user calls el.style.setProperty() -> webidl2js layer calls el.styleImpl.setProperty() -> el.styleImpl._style.setProperty() -> el.styleImpl._style[property] = value -> generated code in properties/* installed on the prototype -> el.styleImpl._style._setProperty()
• user calls el.style.backgroundPosition = value -> webidl2js layer calls el.styleImpl.backgroundPosition = value -> el.styleImpl._style.backgroundPosition = value -> generated code in properties/* -> el.styleImpl._style._setProperty()

This architecture is not much better. It still doesn't match the spec. It introduces a webidl2js layer, but doesn't take full advantage of it, since it has three layers now. (webidl2js wrapper, jsdom CSSStyleDeclarationImpl, and then cssstyle CSSStyleDeclaration.) And it's still using a property descriptor architecture.

The goal I am proposing, per #235:

• user calls el.style.setProperty() -> webidl2js layer calls el.styleImpl.setProperty() -> generated code in properties/* (not on the prototype) -> el.styleImpl._setProperty()
• user calls el.style.backgroundPosition = value -> webidl2js layer calls el.styleImpl.backgroundPosition = value -> jsdom code calls el.styleImpl.setProperty("background-position", value) -> generated code in properties/* (not on the prototype) -> el.styleImpl._setProperty()

This architecture matches the spec, and fits well with the rest of jsdom:

• There is a single impl class for CSSStyleDeclaration, which lives in the cssstyle package.
• That impl class matches the spec. You can line up the public API with those defined in the spec, and for each public API function, you can match it up with the spec's steps.
• Everything related to CSSStyleProperties (i.e., everything relating to creating a JavaScript properties for background-position and backgroundPosition) are handled in the jsdom package, so that the cssstyle package can focus on providing a good implementation of setProperty(), getPropertyValue(), cssText, etc.
• cssstyle now only needs to worry about background-position, not backgroundPosition. (At least in the CSSStyleDeclaration API and all generated code. It could still export a list of pre-computed aliases from its index.js for jsdom to use.)
• cssstyle does not need to worry about stuff that webidl2js can handle, like argument validation, indexed property support, etc.

Note that I propose we start by delegating to generatedProperties[property].set.call(), but over time (in future work) we could simplify that. Hopefully we could remove or reduce layers of generated code, and in particular move away from the property-descriptor architecture. That architecture contains a lot of extra code that isn't needed, especially for properties that aren't shorthands.