Add dev hint about css object being stringified by accident

[Andarist]

No description provided.

[netlify]

Preview Docs

Built with commit 2c61e9d

https://deploy-preview-1219--emotion.netlify.com

[emmatown]

I'm not a big fan of this, while it would help some people, I'm worried that toString will be called for random reasons(iirc react-hot-loader might have done this) and people will open issues with "why is there some random error, i'm not doing what it says i'm doing". This used to be problem with the component selector error, it used to throw on toString but toString was getting called for random reasons and it was expected that it didn't have side effects and didn't throw so the error was moved to serialization.

[Andarist]

Hm, maybe it was called on components because they were components that react hot loader had tried to patch somehow? Not sure why regular object would get stringified by accident 🤔

[emmatown]

I had an idea, what if we returned this message from toString so when people inspect the dom, they'll see the message but toString doesn't have any side effects that could cause confusion if somehow stringified for another reason?

[codecov]

Codecov Report

Merging #1219 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/serialize/src/index.js	100% <100%> (ø)	⬆️
packages/jest-emotion/src/utils.js	98.85% <0%> (+1.14%)	⬆️

[Andarist]

Not sure if I follow - how people inspecting the DOM would see our message?

[emmatown]

Something like this, https://codesandbox.io/s/73kjljkzv6

[Andarist]

Ok, this is clever XD So you would basically like to move the error message from console.error to return?

[emmatown]

Yes.

[changeset-bot]

🦋 Changeset is good to go

Latest commit: c20e955

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Andarist]

@mitchellhamilton I've updated this long-forgotten PR. Should be ready to merge now.

[emmatown]

Could we use react testing library so we're using react-dom here and encounter what will happen in the browser?

[Andarist]

used testing library

[Andarist]

I've adjusted the PR according to your suggestion 👍

I must say though that I'm not entirely sold on that toString trick. Seems for me that often console.error is easier to spot and the stack is attached to it, so it's also easier to find quickly the exact faulty call site. Can't see when this would matter as css opaque object is not intended for anything else other than emotion and it shouldn't be consumed in any other way - so if this gets stringified for any other reason it's immediately breaking a contract and I think we shouldn't care about such possibility, this simply should not be allowed.

[emmatown]

Thanks!!

[Andarist]

@mitchellhamilton what about my reservations about just using toString trick without console.error? Should we switch back to console.error or do you think this is fine as is?

[emmatown]

I think it's fine as it is. Based on the "why is there [object Object] on my element" issues, I think it'll address the problem because the first thing people will probably do if a style isn't being applied is inspect the element where they'll see this text. Also, I want to avoid toString functions that log/have visible side effects in general because we've been burned by it in the past when the error about not using the babel plugin when using component selectors showed up when people weren't even interpolating it because it was in the toString(it's since been moved to serialize)

[mycarrysun]

It might be helpful to add some extra note in the error message that you can use css that is exported from core instead of adding it to the css prop on an element. I had to do it this way to use React.cloneElement - I was able to accomplish adding custom styles and combining with original styles on the element.

[Andarist]

@mycarrysun could you elaborate more on this? I'm not exactly sure what you are proposing

[mycarrysun]

@Andarist the goal of my feature was to add styles to all children but also allow the children to declare their own styles if necessary. Here is the component:

import { css as createEmotionClassName } from 'emotion';

export function FormAccessibleWrapper({
  children,
}) {
  return React.Children.map(children, (child) => {
    const { props: { css: childCss } = {} } = child;

    return React.cloneElement(child, {
      className: createEmotionClassName([
        styles.base,
        includeHoverStyles && styles.hover,
        includeFocusStyles && styles.focus,
        includeActiveStyles && styles.active,
        childCss,
      ]),
    });
  });
}

Originally I had tried to do:

export function FormAccessibleWrapper({
  children,
}) {
  return React.Children.map(children, (child) => {
    const { props: { css: childCss } = {} } = child;

    return React.cloneElement(child, {
      css: [
        styles.base,
        includeHoverStyles && styles.hover,
        includeFocusStyles && styles.focus,
        includeActiveStyles && styles.active,
        childCss,
      ],
    });
  });
}

It would be great to know that I needed to use the css function that emotion exports. Took a few tries and I stumbled upon the solution but it wasn't very straightforward based on the error message.

Here is an error message that would be more helpful:

You have tried to use an emotion compiled stylesheet as input to the css prop, please pass this to className instead or compose a new stylesheet by passing it into css()

Might be a better way to word the "passing it into css()" but hopefully you get my drift