Add a babel-macro #2032

lucleray · 2018-09-27T20:29:21Z

Since #1256 seems to be not active anymore, I created a new PR to add babel-macro to styled-components.

Macro
Import visitors from babel-plugin-styled-components
Tests
Flow types (enough for flow check to pass)
Typescript types
Export as styled-components/macro
Implement visitors from babel-plugin-styled-components

mxstbr

Don't have time to review right now but thanks so much for picking this up! I'll defer to @probablup for an actual code review, I'd love to land this in v4 to be compatible with create-react-app v2!

quantizor · 2018-09-28T04:47:04Z

Let's add a test case to verify that all helpers exported by src/index.js have a matching implementation in the macro so if we add or change something in the future, we'll be notified via tests if we're missing something.

lucleray · 2018-09-28T09:37:38Z

@probablyup

Let's add a test case to verify that all helpers exported by src/index.js have a matching implementation in the macro so if we add or change something in the future, we'll be notified via tests if we're missing something.

I've added a test for that here :

styled-components/src/macro/test/macro.test.js

Lines 64 to 70 in 7992c61

    
           test('should allow all helpers exported from styled-components', () => { 
        
             const styledExports = Object.keys(styledTopLevel).filter( 
        
               helper => helper !== '__esModule' 
        
             ) 
        
             expect(styledExports).toEqual(allowedImports) 
        
           })

lucleray · 2018-09-28T09:56:12Z

With the current implementation, that's the way it works :

import styled, { css, keyframes, injectGlobal, ThemeProvider } from 'styled-components/macro'

// do things with imports
css`...`
keyframes`...`
injectGlobal`...`
styled.div`...`
<ThemeProvider />

It will get transpiled to something like this (just focusing on what the macro does, not the other babel plugins) :

import _styled, { 
  css as _css,
  keyframes as _keyframes,
  injectGlobal as _injectGlobal,
  ThemeProvider as _ThemeProvider 
} from 'styled-components'

_css() 
_keyframes()
// lighter precompiled versions

_injectGlobal`...`
// not modified because babel-plugin-styled-components keeps it like this

_styled.div.withConfig({ displayName: '' })()
// adds withConfig (displayName, ...) and use the lighter version

<_ThemeProvider /> 
// kept like this, just moved the import from styled-components/macro to styled-components

⚠️I kept injectGlobal here because I created a branch from master but it's exactly the same with createGlobalStyle

lucleray · 2018-09-28T10:12:32Z

@probablyup
Could you let me know if the current behaviour of the macro is good on your side ?

lucleray · 2018-09-29T00:15:22Z

I added just enough flow types for flow check to pass. I'm not sure we need more ?

If someone knows Flow, I'd love some advice on the flow types in this PR 😬

Otherwise, I think the PR is ready for review 😀

quantizor · 2018-09-29T14:34:41Z

@lucleray could you provide me with a testing guide? I don't use CRA so not sure how to go about making sure this works (and continues to work)

lucleray · 2018-09-29T22:56:08Z

@probablyup

I created an example repo here : https://github.com/lucleray/cra-styled-components

First, you need to set it up.

Clone this PR, yarn install, yarn build and yarn link
Clone the example repo, yarn install and yarn link styled-components

Once this is set up, you can test if the macro does the right thing.

I added two styled components in src/App.js : https://github.com/lucleray/cra-styled-components/blob/4883352b32a9adfbcfad02710b79152a7007deae/src/App.js#L4-L19

Run cra :

To run cra in dev mode : yarn start
To run cra build : yarn build

Then you can check that the code is correctly transpiled :

In dev mode, I use the console's sources tab
When it's build, I look in the build/ directory

I look for the main chunk (static/js/main.<...>.chunk.js) and search for StyledDiv in the code. I see something like this :

var StyledDiv = styled_components__WEBPACK_IMPORTED_MODULE_5__["default"].div.withConfig({
  displayName: "App__StyledDiv",
  componentId: "sc-19hhnfk-0"
})(["background:red;padding:5px;margin-top:1em;border-radius:5px;"]);

Similarly, I search for StyledNoMacroDiv :

var StyledNoMacroDiv = styled_components__WEBPACK_IMPORTED_MODULE_6__["default"].div(_templateObject());

lucleray · 2018-09-29T23:00:04Z

The example repo was quite easy to setup :

npx create-react-app@next --scripts-version=@next cra-styled-components
cd cra-styled-components
yarn add styled-components

And this is all you need to do to start using CRA + styled-components once this macro will be added to styled-components 🔥

quantizor

The base of this also needs to be switched to "develop"

quantizor · 2018-10-01T03:58:42Z

src/macro/index.js

+import templateLiteral from 'babel-plugin-styled-components/lib/visitors/templateLiterals/index'
+import pureAnnotation from 'babel-plugin-styled-components/lib/visitors/pure'
+
+const taggedTemplateImports = ['css', 'keyframes', 'injectGlobal', 'default']


need to add createGlobalStyle and injectGlobal is no longer a thing in v4 so it can be removed

quantizor · 2018-10-01T03:59:15Z

src/macro/index.js

+  'keyframes',
+  'injectGlobal',
+  'isStyledComponent',
+  'consolidateStreamedStyles',


this is removed

quantizor · 2018-10-01T04:00:13Z

src/macro/index.js

+          // merge config into the state
+          const stateWithOpts = { ...state, opts: config }
+          // run babel-plugin-styled-components appropriate visitors
+          minify(t)(templatePath, stateWithOpts)


Is there a way to reuse the setup from the babel plugin wholesale instead of having to recompose all of these? I can't see us maintaining this properly if it isn't more automatic. It's also a bit more complicated as of 1.8 since we're handling call expressions too

That sounds like a good idea 😀

lucleray · 2018-10-05T10:14:11Z

@strayiker @mxstbr
Ok! I'll keep the code here.

Waiting for @probablyup's answer :
#2032 (comment) : "should babel-plugin-styled-components dependency be a peerDependency or a dependency ?" 🙂

quantizor · 2018-10-05T12:54:07Z

Yes it should be a peer dep

…

On Fri, Oct 5, 2018 at 6:14 AM Luc ***@***.***> wrote: @strayiker <https://github.com/strayiker> @mxstbr <https://github.com/mxstbr> Ok! I'll keep the code here. Waiting for @probablyup <https://github.com/probablyup>'s answer : #2032 (comment) <#2032 (comment)> : "should babel-plugin-styled-components dependency be a peerDependency or a dependency ?" 🙂 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2032 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1n1jYF_qLkaDKO7sgag7-5W_cZ4Aks5uhzD1gaJpZM4W9Z1O> .

lucleray · 2018-10-06T01:52:55Z

@probablyup

Yes it should be a peer dep

What is the reason for that ?

Emotion is listing it as a dependency : https://github.com/emotion-js/emotion/blob/f8e795fe77d601b718ea4da4a906f820599c20a5/packages/css/package.json#L12

quantizor · 2018-10-06T02:40:11Z

So it dedupes properly if another third party library adds it

…

On Fri, Oct 5, 2018 at 9:53 PM Luc ***@***.***> wrote: @probablyup <https://github.com/probablyup> Yes it should be a peer dep What is the reason for that ? Emotion is listing it as a dependency : https://github.com/emotion-js/emotion/blob/f8e795fe77d601b718ea4da4a906f820599c20a5/packages/css/package.json#L12 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2032 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1jHZuPh0wS3_6k-qM-lzeJunHUeKks5uiAz7gaJpZM4W9Z1O> .

lucleray · 2018-10-06T03:09:08Z

@probablyup
Ok, I moved it to peerDependency here : 5cef311

I'm still not sure the dedup is more important than the ease of use/simplicity though 🤔

mxstbr · 2018-10-06T13:52:24Z

I'm still not sure the dedup is more important than the ease of use/simplicity though 🤔

It won't be obvious for create-react-app users that they have to import styled from 'styled-components/macro' anyway, which means we will need to add this to the documentation (once as its own item "Usage with create-react-app" and once in the API section somehow) anyway—so we might as well add a second step to the documentation that says that you first have to install the Babel plugin.

In fact, this might be a compelling reason to move the macro to the Babel plugin instead after all (sorry 🙈). Imagine the docs as it is right now:

## Usage with create-react-app

First, install the Babel plugin:

$ npm i babel-plugin-styled-components

Then use the Babel macro:

import styled from 'styled-components/macro';

It's pretty unobvious that you need to install the plugin alongside before being able to use the macro mode. Contrast with this explanation:

## Usage with create-react-app

First, install the Babel plugin:

$ npm i babel-plugin-styled-components

Then use the Babel macro:

import styled from 'babel-plugin-styled-components/macro';

That's more obvious, if a bit more typing in the import statement.

Not sure what's better or worse, what do you think?

lucleray · 2018-10-06T15:19:40Z

@mxstbr

IMO, the best way experience for users is to just install styled-components and import it through the macro and it works :

## Usage with create-react-app

Install styled-components:

$ npm i styled-components

Use it through the macro:

import styled from 'styled-components/macro';

You don't need to install babel-plugin-styled-components at all ! (That means babel-plugin-styled-components should be a dep and not a peer dep)

One of the reason for babel-plugin-macros to exist as a replacement of babel plugins is (listed in this article) :

[Babel plugins] can lead to confusion because when looking at code in a project, you might not know that there's a plugin transforming that code.

When you need to install babel-plugin-styled-components as a user :

if you import styled from 'styled-components/macro' then it's not explicit why you need babel-plugin-styled-components because it's not in your source code
if you import styled from 'babel-plugin-styled-components/macro' then it's not clear why you need styled-components (if this is what we want to do, then why not use a dedicated package styled-components.macro like this one)

Also, other libraries are doing the same :

styled-jsx => import css, { resolve } from 'styled-jsx/macro', no need to install babel-plugin-syntax-jsx
emotion => import { css } from 'emotion/macro' , no need to install babel-plugin-emotion.

To sum up, this is what I think is best for users :

	description	how it works
Solution 1	babel-plugin-styled-components is a dep (not a peer dep)	`import styled from 'styled-components/macro'`
Solution 2	create a dedicated package `styled-components.macro` (maybe move this one to the styled-components org)	`import styled from 'styled-components.macro'`

mxstbr · 2018-10-06T15:49:51Z

I see, good thinking. I think adding it as a dep is really the best UX.

@probablyup let's put it as a dep with a really lenient version, maybe => 1.0? People should be using it anyway so I don't feel bad about installing it alongside automatically.

gilbsgilbs · 2018-10-06T15:53:33Z

It won't be obvious for create-react-app users that they have to import styled from 'styled-components/macro' anyway

Speaking of this, what will happen if they don't? Will it still work? Work but silently not apply transforms? Throw some error? Is there any error / linting rule one can enable to ensure the macro is consistently imported in CRA instead of the "normal" import? Can we document this if it's not already?

mxstbr · 2018-10-06T15:56:38Z

Speaking of this, what will happen if they don't? Will it still work? Work but silently not apply transforms?

Just like it does today it will work as normally, just without any Babel transforms.

Can we document this if it's not already?

See my reply further up,

which means we will need to add this to the documentation (once as its own item "Usage with create-react-app" and once in the API section somehow) anyway

lucleray · 2018-10-07T04:49:23Z

Speaking of this, what will happen if they don't? Will it still work? Work but silently not apply transforms?

Just like it does today it will work as normally, just without any Babel transforms.

Yes, this is true with the current implementation, unless there is a file with 2 imports like this :

import styled from 'styled-components'
import styled2 from 'styled-components/macro'
// ...

In that case, both styled imports will be transformed.

This is because of the current implementation. When import from 'styled-components/macro' is present in the file, the macro will pass the file to the babel plugin and the babel plugin will transform the whole file, not just locally.

The implementation is like this because we want the macro to be as close to the babel plugin as possible and easy to maintain when the babel plugin is updated. If we wanted to just transform locally, the macro should be aware of styled-components' API and that would be hard to maintain.

For example, if it is used like this :

import styled from 'styled-components'
import styled2 from 'styled-components/macro'

styled2.div`background: red;`

If we want to only transform locally, we need to go up twice the tree to apply the transformation to the thing that matters to styled components, the TaggedTemplateExpression :

styled2 ===(parentPath)===> styled2.div ===(parentPath)===> styled2.div`background: red;`

But this is specific to the default import, so we need to do the same for the other imports (css, keyframes, ...). So we start to implement styled-components' API in the macro itself. That's why the current implementation just pass the whole file to babel plugin and doesn't do things locally.

AFAIK, I can't find a real use case for importing both styled-components and styled-components/macro in the same file, so I think this behaviour is acceptable. What do you think ?

mxstbr · 2018-10-08T07:29:39Z

AFAIK, I can't find a real use case for importing both styled-components and styled-components/macro in the same file, so I think this behaviour is acceptable. What do you think ?

Yeah that's totally fine.

mxstbr

Let's add the Babel plugin back to deps and ship this?! @probablyup

quantizor · 2018-10-08T21:05:10Z

I wish there was a way to have the macro applied automatically without having to specify a different import path...

quantizor · 2018-10-08T21:06:34Z

Maybe in the docs for this we should also recommend setting up aliasing so "styled-components" -> "styled-components/macro"

quantizor · 2018-10-08T21:16:02Z

~~Also it looks like the community convention seems to be name.macro rather than name/macro. Will the former work so people don't get confused?~~ nvm it appears people are publishing entirely separate packages which is kind of meh

quantizor · 2018-10-08T21:37:27Z

Alright it's going in! Thanks for all the amazing work here @lucleray!

mxstbr · 2018-10-08T21:37:36Z

Thank you so much for helping us improve styled-components! Based on our Community Guidelines every person that has a PR of any kind merged is offered an invitation to the styled-components organization—that is you right now!

Should you accept, you'll get write access to the main repository. (and a fancy styled-components logo on your GitHub profile!) You'll be able to label and close issues, merge pull requests, create new branches, etc. We want you to help us out with those things, so don't be scared to do them! Make sure to read our contribution and community guidelines, which explains all of this in more detail. Of course, if you have any questions just let me know!

gilbsgilbs · 2018-10-08T23:07:39Z

Thanks for the work on this PR.

I wish there was a way to have the macro applied automatically without having to specify a different import path...

Maybe in the docs for this we should also recommend setting up aliasing so "styled-components" -> "styled-components/macro"

It's not aliasing, but no-restricted-imports can at least prevent mistakes. Does this miss any import?

rules:
  no-restricted-imports:
    - error
    - paths:
      - name: 'styled-components'
        message: 'Please import from styled-components/macro.'
      patterns:
        - '!styled-components/macro'

I find it more explicit and intended than aliasing, however I'd be interested in knowing how you would implement aliasing. Using resolve.alias?

lucleray · 2018-10-09T08:09:34Z

Happy to see this merged 😀

@gilbsgilbs
If we want to add a warning/aliasing, it should only be for people who are using babel-plugin-macros, and that seems very tricky to detect, so I'm not sure that would work 🤔

gilbsgilbs · 2018-10-09T08:21:23Z

If we want to add a warning/aliasing, it should only be for people who are using babel-plugin-macros, and that seems very tricky to detect, so I'm not sure that would work thinking

Definitely. It was rather a suggestion for the documentation. I was saying that I prefer having ESLint rules rather than aliasing which I find too implicit and "magic" (even though I'd be interested in knowing how aliasing could work here). I'm sure we'll all forget to import from the macro at some point and it would be unnoticeable otherwise.

mxstbr reviewed Sep 27, 2018

View reviewed changes

mareksuscak mentioned this pull request Sep 27, 2018

babel-plugin-macros #1200 #1256

Closed

quantizor mentioned this pull request Sep 29, 2018

Support babel-plugin-macros styled-components/babel-plugin-styled-components#142

Closed

quantizor reviewed Oct 1, 2018

View reviewed changes

quantizor mentioned this pull request Oct 2, 2018

Use with pre-eject create-react-app@2.0 styled-components/babel-plugin-styled-components#168

Closed

lucleray changed the base branch from master to develop October 2, 2018 11:16

lucleray added 16 commits October 2, 2018 13:39

create macro

63fa1e5

add sourceType module when using babel.template

7250f4a

babel.types needs to be passed down to visitors

c198695

add tests

e570e6d

add pureAnnotation visitor from babel-plugin-styled-components

65ad441

add test with require()

fa76559

export 'styled-components/macro'

7f26aa5

add css to macro

698b6bd

add keyframes to macro

924d390

update test snapshots

217ae1d

allow to import all helpers from src/index.js

57fdc0d

add error when tags are not used with template literals

eae70c3

add test : should allow all helpers exported from styled-components

a4188f0

add injectGlobal to taggedTemplateImports

f3e6707

add missing tests and refactor tests

6fc55be

add typescript types

526a72c

move babel-plugin-styled-components to peerDependencies

5cef311

fix tests : add babel-plugin-styled-components to devDependencies

d0c4774

mxstbr approved these changes Oct 8, 2018

View reviewed changes

move babel-plugin-styled-components to deps, with a lenient version

e578ee4

quantizor added 3 commits October 8, 2018 16:28

Merge remote-tracking branch 'origin/develop' into HEAD

cf18ecd

add macro to files

5378847

add changelog entry

cd0b34f

quantizor merged commit 4e40f85 into styled-components:develop Oct 8, 2018

quantizor mentioned this pull request Oct 8, 2018

Create a Babel macro #1200

Closed

lucleray mentioned this pull request Nov 26, 2018

Make the css prop work with the macro! 🎉 styled-components/babel-plugin-styled-components#181

Merged

Add a babel-macro #2032

Add a babel-macro #2032

Conversation

lucleray commented Sep 27, 2018 • edited

mxstbr left a comment

Choose a reason for hiding this comment

quantizor commented Sep 28, 2018

lucleray commented Sep 28, 2018

lucleray commented Sep 28, 2018 • edited

lucleray commented Sep 28, 2018 • edited

lucleray commented Sep 29, 2018 • edited

quantizor commented Sep 29, 2018 • edited

lucleray commented Sep 29, 2018

lucleray commented Sep 29, 2018

quantizor left a comment

Choose a reason for hiding this comment

quantizor Oct 1, 2018

Choose a reason for hiding this comment

quantizor Oct 1, 2018

Choose a reason for hiding this comment

quantizor Oct 1, 2018

Choose a reason for hiding this comment

lucleray Oct 2, 2018

Choose a reason for hiding this comment

lucleray commented Oct 5, 2018

quantizor commented Oct 5, 2018 via email

lucleray commented Oct 6, 2018

quantizor commented Oct 6, 2018 via email

lucleray commented Oct 6, 2018

mxstbr commented Oct 6, 2018

lucleray commented Oct 6, 2018

mxstbr commented Oct 6, 2018

gilbsgilbs commented Oct 6, 2018

mxstbr commented Oct 6, 2018

lucleray commented Oct 7, 2018 • edited

mxstbr commented Oct 8, 2018

mxstbr left a comment

Choose a reason for hiding this comment

quantizor commented Oct 8, 2018

quantizor commented Oct 8, 2018

quantizor commented Oct 8, 2018 • edited

quantizor commented Oct 8, 2018

mxstbr commented Oct 8, 2018

gilbsgilbs commented Oct 8, 2018

lucleray commented Oct 9, 2018 • edited

gilbsgilbs commented Oct 9, 2018 • edited

lucleray commented Sep 27, 2018 •

edited

lucleray commented Sep 28, 2018 •

edited

lucleray commented Sep 28, 2018 •

edited

lucleray commented Sep 29, 2018 •

edited

quantizor commented Sep 29, 2018 •

edited

lucleray commented Oct 7, 2018 •

edited

quantizor commented Oct 8, 2018 •

edited

lucleray commented Oct 9, 2018 •

edited

gilbsgilbs commented Oct 9, 2018 •

edited