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

Program visitor not being run for styled-components macro #121

Open
quantizor opened this issue Jun 19, 2019 · 11 comments · Fixed by #156
Open

Program visitor not being run for styled-components macro #121

quantizor opened this issue Jun 19, 2019 · 11 comments · Fixed by #156

Comments

@quantizor
Copy link
Contributor

Relevant babel plugin code: styled-components/babel-plugin-styled-components@e3829d2#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 babel-plugin-styled-components@1.10.1, 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...

@kentcdodds
Copy link
Owner

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 :(

@quantizor
Copy link
Contributor Author

quantizor commented Jun 20, 2019

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?

@kentcdodds
Copy link
Owner

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

@bySabi
Copy link

bySabi commented Feb 8, 2020

@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: #31

kentcdodds pushed a commit that referenced this issue Jun 14, 2020
* use babel traverse

* remove log statement

* remove semicolon

Closes #121
@github-actions
Copy link

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@conartist6
Copy link
Collaborator

conartist6 commented Dec 7, 2020

@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
Copy link
Collaborator

conartist6 commented Dec 7, 2020

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
Copy link
Collaborator

conartist6 commented Dec 7, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants