jsdom / cssstyle

A Node.js implementation of the CSS Object Model CSSStyleDeclaration interface
MIT License
107 stars 70 forks source link

fix: convert value to DOMString #130

Closed cdoublev closed 3 years ago

cdoublev commented 3 years ago

Fix #129.

codecov-commenter commented 3 years ago

Codecov Report

Merging #130 (3b74e06) into master (b527ed7) will increase coverage by 43.13%. The diff coverage is 94.82%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #130       +/-   ##
===========================================
+ Coverage   37.39%   80.52%   +43.13%     
===========================================
  Files          87       87               
  Lines        1182     1171       -11     
  Branches      227      216       -11     
===========================================
+ Hits          442      943      +501     
+ Misses        633      180      -453     
+ Partials      107       48       -59     
Impacted Files Coverage Δ
lib/properties/borderColor.js 90.90% <ø> (+90.90%) :arrow_up:
lib/properties/borderWidth.js 78.26% <ø> (+78.26%) :arrow_up:
lib/parsers.js 82.53% <86.95%> (+1.88%) :arrow_up:
lib/CSSStyleDeclaration.js 94.64% <100.00%> (+3.46%) :arrow_up:
lib/properties/backgroundPosition.js 66.66% <100.00%> (+66.66%) :arrow_up:
lib/properties/borderSpacing.js 38.88% <100.00%> (+38.88%) :arrow_up:
lib/properties/borderStyle.js 90.90% <100.00%> (+90.90%) :arrow_up:
lib/properties/clip.js 84.00% <100.00%> (+84.00%) :arrow_up:
lib/properties/flex.js 85.71% <100.00%> (+85.71%) :arrow_up:
lib/properties/flexBasis.js 90.90% <100.00%> (+90.90%) :arrow_up:
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b527ed7...3b74e06. Read the comment docs.

cdoublev commented 3 years ago

Unfortunately, despite my successive attemps, this PR will not be enough in all cases. Eg. with style.borderStyle = { toString: () => 'solid' }, the setter for border-style will not validate the value.

I feel like this package (and the issue related to this PR) requires a pretty large overhaul. IMO parsers.valueType could be removed, as well as isValid, which are almost always equivalent to parseTypeA(val) ?? parseTypeB(val) ?? ... and parseType(val) !== undefined, respectively, and each implemented property setter could be transformed to convert value to a DOM String before being passed to the custom setter, if any, ie. set: () => transformedSet(toDOMString(val)).

If any of the maintainers think spending more time on this is pointless because of a lack of their time, please let me know!

cdoublev commented 3 years ago

Finally found a way to fix style.borderStyle = { toString: () => 'solid' }.

But one notable change is that implemented properties are no longer parsed and generated with babel. I believe this was only required by cssom in order to be run in a browser. I didn't bother to install jsdom to run its tests with this change.

Why do I need this problem to be fixed?

I maintain a JS library for animating HTML elements. In my jest tests, which depends on cssstyle via jsdom, many different CSS properties have their value evaluated. In summary, I want to make a change that transforms the final statement animated.style.property = stringValue into animated.style.property = { toString: () => stringValue }, but that makes those tests failing.

I can redefine the animated properties descriptors, so that I don't have to wait for a fix to this package and a new version of jsdom andjest:

// setupFile.js

const createSetter = initialSetter => function set(v) {
    return initialSetter.bind(this)(String(v))
}
const descriptor = Object.getOwnPropertyDescriptor(CSSStyleDeclaration.prototype, 'backgroundColor')

// descriptor.set = createSetter(descriptor.set) // Doesn't work
Object.defineProperty(
    CSSStyleDeclaration.prototype,
    'backgroundColor',
    { ...descriptor, set: createSetter(descriptor.set) })

// Repeat for each animated property name used in tests

This library does not have any test setup file at this time. It's better like that and without having to update such file every time a test involving a new CSS property needs to be added.

cdoublev commented 3 years ago

Superseded by #140.