babel-plugin-macros icon indicating copy to clipboard operation
babel-plugin-macros copied to clipboard

Program visitor not being run for styled-components macro

Open quantizor opened this issue 5 years ago • 11 comments

Relevant babel plugin code: https://github.com/styled-components/babel-plugin-styled-components/commit/e3829d28f2b460e097f1499f6091a52b667ef1b4#diff-1fdf421c05c1140f6d71444ea2b27638L13

Relevant macro code: https://github.com/styled-components/styled-components/blob/master/packages/styled-components/src/macro/index.js

Basically when I switched the root level JSXAttribute visitor into one that's a subtraversal of Program (necessary to create the component AST the other visitors then further modify) the part of the babel plugin inside the Program visitor stopped working for macro users.

Here's a repro sandbox: https://codesandbox.io/embed/magical-brook-ctyxs (styled-components@* and [email protected], the text should be green.)

To be totally honest I generally am not great at writing Babel code so maybe I'm just doing something wrong? But it's odd because I have a bunch of tests in the repo to make sure the transforms happen properly and they do seem to function as expected, it's just this Program subtraversal that isn't working for the macro use case...

quantizor avatar Jun 19 '19 04:06 quantizor

Hi @probablyup!

I don't know what's up and I'm afraid I don't have the bandwidth to look into it right now :(

kentcdodds avatar Jun 19 '19 21:06 kentcdodds

One interesting thing is @jamesknelson was looking into this and switching from state.file.path.traverse() to using @babel/traverse inside the macro made it work. Maybe switching to that inside this library during applyMacros might work?

quantizor avatar Jun 20 '19 00:06 quantizor

I'm good with that solution is someone wants to make a pull request.

kentcdodds avatar Jun 20 '19 03:06 kentcdodds

@probablyup , I am trying to make a macro for this babel plugin: https://github.com/insin/babel-plugin-react-html-attrs/blob/master/lib/index.js ... and it is not clear to me if it can be done or not.

I think it's a problem similar to yours, what do you think?

Here it has also been mentioned in: https://github.com/kentcdodds/babel-plugin-macros/issues/31

bySabi avatar Feb 08 '20 13:02 bySabi

:tada: This issue has been resolved in version 3.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] avatar Nov 26 '20 07:11 github-actions[bot]

@probablyup Sorry to bother you. It looks like the fix implemented for this issue caused a regression for a different issue. I want to make sure we don't cause the issue described here again as we try to fix the regression, but unfortunately the code sandbox link your provided when you created this issue is now gone. Do you happen to have a copy of it, or can you help me understand what was happening?

conartist6 avatar Dec 07 '20 04:12 conartist6

Actually I think I understand what you were talking about now. I see where you changed the plugin to traverse inside of the Program visitor. I'll dig into it some more tomorrow.

conartist6 avatar Dec 07 '20 04:12 conartist6

OK yeah I am now pretty sure the fix for this issue is wrong and will I believe need to be reverted. I can help you look again at the problem described here and determine its root cause and the correct fix.

conartist6 avatar Dec 07 '20 05:12 conartist6

I theorize that your problem is on this line. I'll have to dig to confirm but I suspect you shouldn't requeue something that hasn't been queued yet? The main traversal is still on the program node and you're requeueing one of its children. I still don't quite understand why it would interact with macros. I'll have to take a peek into the Babel code tomorrow to get those answers.

conartist6 avatar Dec 07 '20 06:12 conartist6

It seems that path.requeue() inserts the node into multiple traversal contexts, where a context consists of { parentPath, scope, state, opts }. That makes sense I guess, it's expected to be used when the underlying data has changed, in which case everyone doing a traversal would need to know. But also there is this line. That looks like a good candidate for how parts of your traversal may have gone missing.

conartist6 avatar Dec 07 '20 14:12 conartist6

Can you try dropping the call to requeue entirely. I do not think you need it as you are altering the body of the program which has yet to be traversed. You can test it with the 2.x version of plugin-macros right?

conartist6 avatar Dec 07 '20 14:12 conartist6