giraud / bs-css

Statically typed DSL for writing css in reason.
ISC License
586 stars 101 forks source link

Update to emotion 11, add react-dom dev dependency #229

Closed TomiS closed 3 years ago

TomiS commented 3 years ago

This PR explores the possibility to update to Emotion 11, see issue here. Not ready for merge!!!

It installs and builds but when I run the test application there are runtime errors such as

Using kebab-case for css properties in objects is not supported. Did you mean counterReset?

and

emotion-serialize.browser.esm.js:58 Uncaught Error: You seem to be using a value for 'content' without quotes, try replacing it with `content: '"linear-gradient(45deg, #FF0000 0, #0000FF 100%)"'`

I'm not sure if these are problems with bindings or something else. I also had to add a dev dependency to react-dom to make it run at all.

idkjs commented 3 years ago

I will take a look at this. Thanks @TomiS.

idkjs commented 3 years ago

Everything in bs-css-emotion/Test.re works except for these lines which are throwing the errors in the browser:

https://github.com/reasonml-labs/bs-css/blob/8bc9dd7482839dd634f37ad0a733b69603d975ef/bs-css-emotion/example/Test.re#L1273-L1300

If you comment them out,everything else seems to work.

Errors are:

// first div

Uncaught Error: You seem to be using a value for 'content' without quotes, try replacing it with `content: '"linear-gradient(45deg, #FF0000 0, #0000FF 100%)"'`
// second div
Uncaught Error: You seem to be using a value for 'content' without quotes, try replacing it with `content: '"open-quote "foo" close-quote"'`
idkjs commented 3 years ago

Looks like the issue is quotes vs backticks in the bindings. I have asked the question on the forum here -> https://forum.rescript-lang.org/t/quotes-vs-backticks-in-bindings/1011. Will come back if I get an answer I can use.

lucasweng commented 3 years ago

Hi @TomiS,

The kebab-case error has been fixed by https://github.com/reasonml-labs/bs-css/commit/6403cc39ab3ce64701aeb34e1cb33e9218716204. That error occurred because the css property wasn't written in camel case. (The Css.style uses emotion's object styles under the hood)

As for the errors about using a value for 'content' without quotes, after looking into emotion-serialize.browser.esm.js, I found a piece of code incorrectly throwing errors on gradient values in dev

https://github.com/emotion-js/emotion/blob/06b314725db578439181331691976beb41203cf5/packages/serialize/src/index.js#L63-L98

if (process.env.NODE_ENV !== 'production') {
  let contentValuePattern = /(attr|calc|counters?|url)\(/
  let contentValues = [
    'normal',
    'none',
    'counter',
    'open-quote',
    'close-quote',
    'no-open-quote',
    'no-close-quote',
    'initial',
    'inherit',
    'unset'
  ]

  // ...

  processStyleValue = (key: string, value: string) => {
    if (key === 'content') {
      if (
        typeof value !== 'string' ||
        (contentValues.indexOf(value) === -1 &&
          !contentValuePattern.test(value) &&
          (value.charAt(0) !== value.charAt(value.length - 1) ||
            (value.charAt(0) !== '"' && value.charAt(0) !== "'")))
      ) {
        throw new Error(
          `You seem to be using a value for 'content' without quotes, try replacing it with \`content: '"${value}"'\``
        )
      }
    }

The contentValuePattern regex is missing the gradient data types (a patch PR has just merged so we may need to wait for emotion's next release and see if that fix works 😅 )

For testing, you could try running NODE_ENV=production yarn dev in bs-css-emotion, and the test application would work fine.

Edit: The content: '"open-quote "foo" close-quote"' error is caused by the same piece of code above, so we may need to wait for another release

dylanarmstrong commented 3 years ago

Hey, I've opened a PR against this PR that rebases onto master again to fix conflicts and have the newer changes.

lucasweng commented 3 years ago

Hi @TomiS, thanks for spending time on this! The newly released @emotion/serialize@1.0.2 no longer causes the using a value for 'content' without quotes errors.

I've tried reinstalling @emotion/css, @emotion/react on the emotion11 branch, and the test application is working now.

giraud commented 3 years ago

great news! can this pr be resolved before reintegration ?

TomiS commented 3 years ago

Sorry for the late answer

@dylanarmstrong I cannot see your PR anywhere. Does it still exist?

dylanarmstrong commented 3 years ago

Hey @TomiS, here it is: https://github.com/TomiS/bs-css/pull/2

TomiS commented 3 years ago

@dylanarmstrong Thanks (Weird I was not able to find it, maybe I just cannot use github :p )

Merged. Let's see how it goes.

giraud commented 3 years ago

is react dependency really mandatory for keyframes ? I spent time removing all react dependencies in bs-css-emotion and this is canceling all that work.

TomiS commented 3 years ago

@giraud Hmm, actually might not be. Let me check.

TomiS commented 3 years ago

Yeah. There indeed was a keyframes function in @emotion/css too. I think I just assumed there wasn't because some part of emotion documentation used the one in @emotion/react

Anyway. I removed the react and @emotion/react dependencies. The emotion test page builds without errors in console now and looks right for what I can tell.

giraud commented 3 years ago

@TomiS Great!!! but I still see a react-dom in devDependencies

TomiS commented 3 years ago

@giraud Now that's removed too. Truthfully speaking, I'm not sure if that's needed or not. But I trust you are :)