fix(Fela): prevent styles crash on invalid CSS property provided

[kuzhelov]

Problem

TL;DR

When, for some reason, for CSS property provided a value that ends with opening bracket (or, in general, a value with invalid braces sequence), this results in shadowing all the subsequent Fela styles added to the Fela's rules <style /> DOM element.

This problem shows up quite often while playing with docs, as it is typical situation when not fully-provided variable values are applied.

---

Specifically, suppose that we have the following style object defined for our component:

const style = (props) => {
   ...
   color: 'rgba('
}

When component is rendered, its styles are transformed into class declarations and added to Fela's <style> DOM element on the page:

<style>
   .a{ display: block; color: rgb( } .b{ display: inline; color: red } ...
</ style>

The problem is that invalid sequence of brackets (that was provided as part of the component's CSS property value) results in browser engine ignoring the rest of the <style /> element content - so that, essentially, all subsequent classes are like not declared.

<style>
    .a{ display: block; color: rgb(       // everything that goes next is ignored by browser
</ style>

This results in absence of styling for components that were relying on these classes:

<!-- oops, styles WON'T be applied -->
<div class="b">
   ...
</div>

Solution

Proposed solution is to introduce plugin that will be responsible for ignoring CSS properties that have improper braces sequence as their value. We can extend this functionality to handle other cases if necessary - till we won't find a better solution (problem will be fixed on the Fela's side, new widely-used plugin will be introduced for handling these cases, etc).

[levithomason]

describe('felaSanitizeCssPlugin') please for consistency and readability in output.

[levithomason]

Code reuse is great, however, it seems the indirection is more hurt than help here. You need to scroll 30 lines to find out what this is doing vs a more simple approach:

expect(sanitize({ top: 0 })).toEqual({ top: 0 })

This is easier to read and maintain.

[codecov]

Codecov Report

Merging #65 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   85.92%   85.92%           
=======================================
  Files          74       74           
  Lines        1101     1101           
  Branches      216      225    +9     
=======================================
  Hits          946      946           
  Misses        149      149           
  Partials        6        6

---

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 5047ec4...7ad1c71. Read the comment docs.

[levithomason]

To the last point, I think having simple objects here would help understanding. It is not clear what the test is doing from looking at it. I need to scroll back and forth, memorize two object shapes, and then picture two different merged shapes in my mind to see the assertion.

I think having the objects in simple plain form is more ideal for testing purposes.

Feedback?

[kuzhelov]

sure, will do this way, agree

[levithomason]

Would encourage less abstraction in testing. Consider this comparison:

const validStringStyle = {
  display: 'block',
  margin: '0 0 0 0',
}

const invalidCssValue = 'rgba('

test('should skip excluded CSS props', () => {
  const cssPropertyToExclude = 'myProperty'

  const sanitize = sanitizeCss({
    skip: [cssPropertyToExclude],
  })

  const style = {
    validStringStyle,
    [cssPropertyToExclude]: invalidCssValue,
  }

  expect(sanitize(style)).toEqual(style)
})

Same without abstraction:

test('should skip excluded CSS props', () => {
  const sanitize = sanitizeCss({ skip: ['myProperty'] })

  const style = {
    validStringStyle: { display: 'block', margin: '0 0 0 0' },
    myProperty: 'rgba(',
  }

  expect(sanitize(style)).toEqual(style)
})

One or two non-dry properties are OK if the trade off is readability. Suggest always prioritizing "code is for humans" above the most optimal solution.

[kuzhelov]

yes, totally valid point, will rework

[levithomason]

Could we get an inline comment here as to what this is trying to achieve?

[kuzhelov]

sure, will add this

[levithomason]

Features look good. Would like to try and simplify the tests and logic to be easier to follow if we can.

Merge at will if you feel it is too critical.