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

Emotion Babel Macro duplicating nodes #726

Closed
mattfysh opened this issue Jun 15, 2018 · 3 comments · Fixed by #730
Closed

Emotion Babel Macro duplicating nodes #726

mattfysh opened this issue Jun 15, 2018 · 3 comments · Fixed by #730

Comments

@mattfysh
Copy link

mattfysh commented Jun 15, 2018

  • emotion version: 9.2.3
  • react version: N/A

Relevant code.

// typography.js
import { css } from 'emotion/macro'

const normal = css`
  font-weight: 400;
`

const demibold = css`
  font-weight: 600;
`

export { normal, demibold }

// compile.js
const fs = require('fs')
const src = fs.readFileSync('typography.js', 'utf-8')
const babel = require('babel-core')
const result = babel.transform(src, {
  filename: '/Users/matt/Projects/sandbox/j2/typo.js',
  plugins: [
    require('babel-plugin-macros'),
    require('@babel/plugin-transform-modules-commonjs'),
  ],
  babelrc: false,
  retainLines: true,
  sourceMaps: 'inline',
})

console.log(result.code)

// <result.code>
"use strict";Object.defineProperty(exports, "__esModule", { value: true });exports.demibold = exports.normal = void 0;var _emotion = require("emotion");

const normal = /*#__PURE__*/(0, _emotion.css)("font-weight:400;");exports.normal = normal;



const demibold = /*#__PURE__*/_css("font-weight:600;");exports.demibold = demibold;

What you did:

Used emotion/macro and babel-plugin-macros to transform code, together with plugin-transform-modules-commonjs

What happened:

The second reference to _css was not replaced with a live reference, causing the output to generate invalid javascript ("_css is not defined" error)

Reproduction:

See above code

Problem description:

Unlike the first reference to the macro import ("css"), the second reference is not replaced with a live reference by the commonjs module transform plugin. This is because when the emotion/macro runs, it is inserting duplicate nodes in the tree - and the module transform plugin is configured to skip nodes it has already visited.

Suggested solution:

Create a new node whenever inserting nodes into the AST, and as an extra verification step, use the checkDuplicateNodes helper:
https://github.com/babel/babel/blob/d383659ca6adec54b6054f77cdaa16da88e8a171/packages/babel-helper-transform-fixture-test-runner/src/index.js#L128

@Andarist
Copy link
Member

I think we just need to clone the returned node here -

return state.emotionImports[runtimeImportPath][importName]
(with conditional t.clone/t.cloneNode - to keep babel@6 and babel@7 compatibility.

Gonna try to find time to prepare PR for this 2morrow.

@Andarist Andarist mentioned this issue Jun 17, 2018
3 tasks
emmatown pushed a commit that referenced this issue Jun 18, 2018
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [ ] Documentation
- [x] Tests
- [x] Code complete

fixes #726
@mattfysh
Copy link
Author

Thanks for the fix @Andarist & also for adding the tests. When do you think the next patch might land on the registry? cheers!

@emmatown
Copy link
Member

Just did a release

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 a pull request may close this issue.

3 participants