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

[babel-plugin-emotion] Rewrites calls to foreign styled #344

Closed
Andarist opened this issue Sep 25, 2017 · 16 comments
Closed

[babel-plugin-emotion] Rewrites calls to foreign styled #344

Andarist opened this issue Sep 25, 2017 · 16 comments

Comments

@Andarist
Copy link
Member

  • emotion version:
    7.3.2
  • react version:
    15.5.4

Relevant code.

import styled from 'styled-components'
export const Button = styled.button`
  border: 0;
`

What you did:

I've installed emotion and added babel-plugin-emotion to my .babelrc.

What happened:

The plugin has rewritten webpack's correct output:

var Button = __WEBPACK_IMPORTED_MODULE_2_styled_components__["c" /* default */].button(_templateObject, ...)

to incorrect

var Button = __webpack_require__.i(__WEBPACK_IMPORTED_MODULE_2_styled_components__["c" /* default */])('button', 'css-ButtonElement-1w1j6fq0', [_templateObject, ...])

Reproduction:

Described above.

Problem description:

It's hard to interoperate between those 2 libs (during migration process in example)

Suggested solution:

Check if the identifier is imported from the emotion package

@tkh44
Copy link
Member

tkh44 commented Sep 25, 2017

You could change styled to emotion or something to avoid this issue.

https://github.com/emotion-js/emotion/blob/master/docs/configurable-imports.md

@Andarist
Copy link
Member Author

Andarist commented Sep 25, 2017

Roger that, but the issue could be avoided altogether by properly checking if styled is comming from the react-emotion package.

Also - Im willing to provide a fix to this (when I have time), posted issue so it wont get forgotten + somebody might be quicker than me in solving this.

@cameron-martin
Copy link
Contributor

I could take a look at doing the above (checking that styled comes from the correct package), if nobody else is currently working on it.

@cameron-martin
Copy link
Contributor

But I should probably wait until #440 is done because it's such a massive change to the plugin.

@Andarist
Copy link
Member Author

Andarist commented Nov 2, 2017

@cameron-martin you working on this would be great! I also think that you should probably wait until the other PR gets merged in to prevent unnecessary conflicts.

@cameron-martin
Copy link
Contributor

This may be slightly tricky because of the amount of different forms people could use to import the library.

import { css } from 'emotion';

css``;
import * as emotion from 'emotion';

emotion.css``;
var emotion = require('emotion');
var css = emotion.css;

css``;
var { css } = require('emotion');
var emotion = require('emotion');

emotion.css``;
import('emotion').then(emotion => emotion.css``);

But possibly going this far isn't necessary since some of these forms aren't supported by the babel plugin at the moment anyway.

@Andarist
Copy link
Member Author

Oh yeah, I think whats only needed for the requested "feature" is supporting only whats supported at the moment. Targeting es imports should be fairly easy, no matter how you use them.

Unfortunately I do not know how to do it in a better way than in pre-Program visitor by looping through nodes and extracting this data in the process.

@cameron-martin
Copy link
Contributor

At the moment, commonjs, amd and es6 imports are 'supported' because it doesn't actually look at the imports, so it'd at least have to replicate that behaviour.

For es6 modules you can use referencesImport as is done in babel-plugin-react-intl.

@Andarist
Copy link
Member Author

Nice, I didn't know that one. A wrapper for using this with a fallback to checking require calls should be possible. Just grab the binding and check its parentPath in the same way referencesImport does it already;

@cameron-martin
Copy link
Contributor

I've started implementing this in it's own npm package, because I want to also use it in my own fork of babel-plugin-react-intl which I am maintaining because my pull request hasn't been merged.

I'm pretty inexperienced at writing babel plugins, so if anything is convoluted then let me know how I can improve it :)

@Andarist
Copy link
Member Author

Really cool work @cameron-martin. I'm really happy that you have decided to make a separate package out of it. I'm gonna certainly use it in the future.

I will try to make some kind of a review this week.

@cameron-martin
Copy link
Contributor

cameron-martin commented Nov 29, 2017

@tkh44 @mitchellhamilton Before I invest too much time into this, can I check that this would be a welcome change?

@emmatown
Copy link
Member

@cameron-martin It would, as long as there's an option to use the imports if they aren't defined in that file.(for the website so there doesn't have to be imports)

@cameron-martin
Copy link
Contributor

cameron-martin commented Nov 29, 2017

This is for the playground, right? We could automatically insert those imports before compiling, then inject a require function into the scope. More concretely, we could change this to something like:

  _compileCode = () => {
    const { code, scope } = this.props
    const codeWithImports = 'import { css } from "emotion";' + code;
    return `(function (${Object.keys(scope).join(',')}, render) {
      ${transform(codeWithImports, {
        presets: ['es2015', 'react', 'stage-1'],
        plugins: [
          [
            'babel-plugin-emotion',
            { autoImportCssProp: false, sourceMap: true }
          ]
        ]
      }).code}
    })`
  }

then change the defintion of scope to being

const scope = {
  logoUrl,
  React,
  require(moduleName) {
    switch(moduleName) {
      case 'emotion': return require('emotion');
      case 'react-emotion': return require('react-emotion');
      // etc
      default: throw new Error('Required an unsupported module');
    }
  }
}

@emmatown
Copy link
Member

We could and I've done that before but it's convenient to transform any call with the correct name and if it's behind an option it shouldn't hurt anything.

@stale
Copy link

stale bot commented Aug 7, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 7, 2018
@stale stale bot closed this as completed Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants