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

fix: convert value to DOMString #130

Closed
wants to merge 1 commit into from
Closed

Conversation

cdoublev
Copy link

@cdoublev cdoublev commented Apr 25, 2021

Fix #129.

@cdoublev cdoublev force-pushed the issue-2 branch 2 times, most recently from c0ec266 to 86a3490 Compare April 29, 2021 12:14
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2021

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%) ⬆️
lib/properties/borderWidth.js 78.26% <ø> (+78.26%) ⬆️
lib/parsers.js 82.53% <86.95%> (+1.88%) ⬆️
lib/CSSStyleDeclaration.js 94.64% <100.00%> (+3.46%) ⬆️
lib/properties/backgroundPosition.js 66.66% <100.00%> (+66.66%) ⬆️
lib/properties/borderSpacing.js 38.88% <100.00%> (+38.88%) ⬆️
lib/properties/borderStyle.js 90.90% <100.00%> (+90.90%) ⬆️
lib/properties/clip.js 84.00% <100.00%> (+84.00%) ⬆️
lib/properties/flex.js 85.71% <100.00%> (+85.71%) ⬆️
lib/properties/flexBasis.js 90.90% <100.00%> (+90.90%) ⬆️
... 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 cdoublev changed the title fix: convert value to String if toString method exist fix: convert value to DOMString Apr 29, 2021
@cdoublev cdoublev force-pushed the issue-2 branch 3 times, most recently from 6e06b71 to d377059 Compare May 1, 2021 07:40
@cdoublev
Copy link
Author

cdoublev commented May 1, 2021

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
Copy link
Author

cdoublev commented May 3, 2021

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 and jest:

// 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 cdoublev force-pushed the issue-2 branch 3 times, most recently from 65dcd48 to 090f3e0 Compare May 3, 2021 15:17
@cdoublev
Copy link
Author

Superseded by #140.

@cdoublev cdoublev closed this May 13, 2021
@cdoublev cdoublev deleted the issue-2 branch June 11, 2021 09:39
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.

Values are not properly converted to DOMString
2 participants