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

10.1.0 jsx implementation causes key errors #2065

Closed
billyjanitsch opened this issue Nov 2, 2020 · 3 comments · Fixed by #2066
Closed

10.1.0 jsx implementation causes key errors #2065

billyjanitsch opened this issue Nov 2, 2020 · 3 comments · Fixed by #2066
Labels

Comments

@billyjanitsch
Copy link
Contributor

billyjanitsch commented Nov 2, 2020

Current behavior:

This component:

import * as React from 'react'

export default function MyComponent() {
  return (
    <div css={{color: 'red'}}>
      <div />
      <div />
    </div>
  )
}

Transpiled with @emotion/babel-preset-css-prop@10.1.0 ({runtime: 'classic'}), using @emotion/core@10.1.0, produces the following key error:

Warning: Each child in a list should have a unique "key" prop. See https://reactjs.org/link/warning-keys for more information.
    at div
    at EmotionCssPropInternal
    at MyComponent

In general, this happens for any element that uses the css prop and has two or more children. If you remove the css prop, or if you remove either or both children, there is no key error.

To reproduce:

See this CodeSandbox. Note that the code is the transpiled output from the Babel preset. (You can't reproduce it on CodeSandbox otherwise because it only manifests with the preset.)

Expected behavior:

There should be no key error.

Environment information:

  • react version: ^17.0.1 (but reproduces in 16.x as well)
  • emotion version: 10.1.0
@billyjanitsch
Copy link
Contributor Author

The issue is with this code path:

// https://github.com/facebook/react/blob/fd61f7ea53989a59bc427603798bb111c852816a/packages/react/src/ReactElement.js#L386-L400
const childrenLength = args.length - 2
if (childrenLength === 1) {
emotionProps.children = args[2]
} else if (childrenLength > 1) {
const childArray = new Array(childrenLength)
for (let i = 0; i < childrenLength; i++) {
childArray[i] = args[i + 2]
}
emotionProps.children = childArray
}
// $FlowFixMe
return React.createElement(Emotion, emotionProps)

In the dev build, React runs key validation in two places: once in the dev implementation of createElement and once in the reconciler. (Note: the Emotion comment links to the prod implementation of createElement, but the dev build actually takes an entirely different code path.)

The createElement validation pass only validates children that are passed as rest arguments, e.g., createElement(type, {children: foo}, bar, baz) will validate bar and baz but not foo. When it validates a node, it marks that validation was performed. If bar/baz are nodes (rather than arrays) then it doesn't need to check keys so it just adds the mark and returns. This is how static children get special-cased to not require keys. The reconciler validation pass then validates only children that don't yet have the mark, which in practice is anything in props.children.

So what's going on here is that Emotion is collapsing the tail args into props.children before calling createElement (see above snippet), so the nodes don't get the early validation mark. This causes them to get validated in the reconciler, by which time they've lost any indication of being static, so from React's POV they require keys.

I don't see a way to fix this other than by reverting the part of #1970 that refactors Emotion's jsx to replicate React's children-collapsing logic. I think the tail args of Emotion's jsx need to get sent to createElement as tail args as well.

@litewarp
Copy link

litewarp commented Nov 2, 2020

So the internals of this are way over my head, but this may help someone like me.

I was having this issue in development and set { runtime: 'automatic' } in my my babel-preset-css-prop config, which cured the development errors.

But, when I tried to deploy as production, I kept getting a _jsx is not defined error.

So for the meantime, I'm just defaulting to {runtime: 'classic'} for production and automatic for development in my babel.config.js

const presets = [
    'next/babel',
    process.env.NODE_ENV === 'production'
      ? '@emotion/babel-preset-css-prop'
      : ['@emotion/babel-preset-css-prop', { runtime: 'automatic' }],
    '@babel/preset-typescript',
  ]

@Andarist
Copy link
Member

Andarist commented Nov 2, 2020

@billyjanitsch thanks for the detailed repro - I've implemented a fix for this and gonna release it shortly.

@litewarp this seems like a separate issue that has been discussed here, please take a look there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants