Add support for babel-macros

[leoasis]

This takes care of at least some of #301.

Wanted to start playing with babel-macros so figured I'd work on adding support here.

Not sure if the usage is as intended, so maybe also pinging @kentcdodds for guidance. Especially for the way I introduce the import of the actual runtime. Had to reach out to program for this.

Also another question I have is when to perform the hoisting, since we don't want that during development I suppose. Maybe use process.env.NODE_ENV? Maybe BABEL_ENV? Maybe babel-macros should have an env that gets passed to the macros?

One more thing, is that this won't catch the css attribute in JSX elements, as babel-macros only gets us the Identifier nodes that are the references from the macro import. I could reach out to the Program node to perform the hoisting in the entire file, but that feels like abusing babel-macros. Maybe babel-macros should also give us

Also needs a lots of testing, will try and see how to add some.

Even if this is still far from what you were thinking, maybe it helps a bit as a starting point for your experiments. Hope it's at least a bit helpful!

[threepointone]

• I'll wait for kent's comments on how best to import the lib

• I think it's fine for the the hoisting to always happen, unless you can think of a problem?

• I expected the hoisting to not happen for css props, and that's fine for now, it shouldn't be a responsibility of this macro (and we'll revisit it when we figure out how to doreader macros for babel-macros)

thanks for the PR, excited to get this in!

[threepointone]

(I also wanted to take this opportunity to use SC/emotion's css parser instead of our homegrown one, but that can wait I suppose)

[leoasis]

Thanks for the quick feedback!

The only concern I can think of for always hoisting is weird behavior when debugging. If some call gets hoisted, you may have problems trying to set a breakpoint in there. Not sure how often that happens, though.

I'm currently working on avoiding traversing by just calling the transformations directly on the paths we're given by babel-macros. This will also make it work if we alias the identifier to something else, like import { css as whatev } from 'glamor/macro'

[threepointone]

iirc the hoisting should only hoist the input object, not the actual css() call, so breakpoints still work, but I'll have to verify. in any case, don't let that stop you. if I had to pick, I'd do it on NODE_ENV, not BABEL_ENV.

[kentcdodds]

Not sure if the usage is as intended, so maybe also pinging @kentcdodds for guidance. Especially for the way I introduce the import of the actual runtime. Had to reach out to program for this.

Yep, that's the way to do that kind of thing. It's not supposed to be easy to modify areas of the code that your macros isn't touching 😉

Also another question I have is when to perform the hoisting, since we don't want that during development I suppose. Maybe use process.env.NODE_ENV? Maybe BABEL_ENV? Maybe babel-macros should have an env that gets passed to the macros?

I think you could just handle that yourself in your macro. Check process.env.NODE_ENV and do whatever you need.

One more thing, is that this won't catch the css attribute in JSX elements, as babel-macros only gets us the Identifier nodes that are the references from the macro import. I could reach out to the Program node to perform the hoisting in the entire file, but that feels like abusing babel-macros.

Yes, this is a limitation and honestly I don't think that there's anything I can do for that... Maybe you could do: <div {...[css]: {}} /> but that'd be way weird...

[leoasis]

Ok I've added support for custom aliases when importing the macro.

I wanted to avoid traversing the paths babel-macros give us to convert the template strings with the visitor (and just perform the path transformation), but I was getting an error with nested calls to css. This was due to the parser grabbing the stubs from the source using start and end positions, which get messed up when doing intermediate transformations (d26497c#diff-e09bf259105b708ada46f5bdeb5f932fR132).

Basically I wanted to do this:

references.css.forEach((path) => {
  if (path.parentPath.type === 'TaggedTemplateExpression') {
    transformToCallExpression(path.parentPath);
  }

  if (parentPath.type === 'CallExpression') {
    hoistCallExpression(parentPath);
  }
});

But had to resort to:

references.css.forEach((path) => {
  path.parentPath.parentPath.traverse(babelPlugin.visitor) // This converts all the subtree (so, including nested expressions)

  if (parentPath.type === 'CallExpression') {
    hoistCallExpression(parentPath);
  }
});

Maybe that was one of the reasons you wanted to use the other parsers? Haven't looked at them yet, but maybe they just manipulate the AST without relying on the source and this won't be a problem.

[kentcdodds]

Ok I've added support for custom aliases when importing the macro.

If I understand you correctly, you want to support:

import {css as myCSS} from 'glamor/macro'

If that's what you mean by this then you should probably know that babel-macros supports this out of the box, you shouldn't have to do anything to handle this.

[leoasis]

@kentcdodds yes! Actually the fact that babel-macros supported it out of the box was the reason I decided to support it. The babel plugin and the hoist plugin were using hardcoded names for the identifier, so that's part of the work I did.

[kentcdodds]

Ah, gotcha 👍 coolio!

[threepointone]

I know this stretches the scope a bit, but would you also like to take a crack at automatically adding labels to the rules (based on var being assigned to, if available, or any other idea you might have)? no worries if you don't, but would make this so compelling to use.

[leoasis]

Yeah sure I can take a stab at it. Do we want to make the current babel plugin to also have this functionality? Also, do we want to add labels also in the object-style calls?

I'm thinking we can do what you suggest and assign a label when we're in a variable declaration. Another one that may be helpful could be adding labels when applied to a JSX element, we can combine the element name with "className", or just the element name, for cases where you inline the css call inside it either with spread or assigned to the className.

Regarding the macro itself, anything else you see missing? What do you think about the approach I had to take because of #312 (comment)?

[threepointone]

sorry have to ignore this PR today, just a bit swamped with work and stuff, will reply over the weekend. but tldr - your approach sounds good.

[leoasis]

Sure! no rush with this!

[leoasis]

I know you may still be busy with some other stuff, this is just for you to read it whenever you have time.

I've added some code to automatically add labels when you have a variable declaration. I tried doing it for css directly inside the className of a jsx element, but realized it wouldn't be very useful as the label would be "div", "span", "a", ie not very helpful. I can add any other strategy we can come up with, I'll think about it a bit more.

Should we disable the labels when compiling for prod?

Also, I want to add some tests for this, and wanted to use, as @kentcdodds suggests in babel-macros, the babel-plugin-tester, but that suggests to use jest so we can benefit of snapshot testing. So I'll wait for your "approval" or not to use these libraries in this repo. If not, I can probably add some regular mocha tests.

Again, no rush on this, will be around when you have time to answer

[kentcdodds]

Hey folks! babel-macros 1.0.0 has been released. You'll probably want to check the docs. Pretty much all you'll need to change is wrap your macro in createMacro from const {createMacro} = require('babel-macros')

Cheers!