Rewrite of the codemod #2

danielberndt · 2018-08-03T15:37:10Z

Hi there!

I eventually found some time to dig into AST transformations, and I had the desire to dig deep 😅

This PR is more or less a complete rewrite, supporting all the old-features, and including many new ones. Just check the __tests__/__fixtures__ folder for some examples.

It also allows you to run tests via npm install && npm test -- --watch for a fast feedback loop while developing.

I haven't tested it on a real code base yet, so consider this more of an WIP...

Here's what this PR should fix from my list:

wont work with glamorous.macro (should be easy to fix)
won't work if you do imports like import g from "glamorous" the codemod expects import glamorous from "glamorous"
you manually need to convert code like <glamorous.div marginTop={5}/> to <div css={{marginTop: 5}}/>
no automatic translation from filterProps/forwardProps to shouldForwardProps (the latter expects a function), most other options (e.g. withProps) are also not directly translatable
forwardProps: ["innerRef"] does not work with emotion
global styles and keyframes can't be automatically translated yet either

danielberndt · 2018-08-03T15:47:45Z

Here's an updated version of the Explorer: https://astexplorer.net/#/gist/7bc4771564a12c9f93c4904b3934aa1c/b8ce61a62c97c913866b42426b24762c4c1d0c83

TejasQ · 2018-08-03T16:57:17Z

Dude. Thank you so much! That was incredible! I was actually planning to set aside time to write up a part of this for you to have a read and possibly learn from. Looks like you did amazing stuff already!

Can I participate in this PR and help with the remaining tasks?

Thanks again for the PR!

danielberndt · 2018-08-03T21:29:40Z

I think I'd like to try continuing with global styles and keyframes. But if you want you can maybe look into improving which properties are actual css properties and which are not here.

e.g <glamorous.Button marginTop={5} disabled={true}/> should not become <button css={{marginTop: 5, disabled: true}}/>. I guess that glamorous is using some external library for getting a list of all valid css attributes. So maybe we can use this here as well.

Also one could attempt to fail the codemod if it encounters filterProps/forwardProps and add some helpful message of which file at which location these are used and what method to use in emotion to replace them. (i.e. asking the user to change their code to const Comp = glamorous(MyComp, {shouldForwardProps: p => ...})({...style}) so that the resulting conversion to emotion will then work... or maybe try to automatically translate it, if it's just a simple list within filterProps

TejasQ · 2018-08-03T22:17:53Z

@danielberndt I'll look at that. Thanks for your contributions so far!

TejasQ

Great stuff! Thanks especially for the tests! They sure help!

I think the implementation needs some docs and now would be a good time to add it so that all of us (especially newcomers) have an idea of what's going on, in case someone wants to learn about codemods.

Knowledge sharing ftw!

Also, I've noticed that changing the ImportDefaultSpecifier in the AST explorer outright removes the default import if it is not gl. 🤔

Since default imports/export specifiers are not dependent on the actual identifiers, I think we can preserve them.

From that explorer link too, removing a css prop from any component results in an error.

TejasQ · 2018-08-03T22:19:48Z

index.js

+module.exports = function(babel) {
+  const {types: t} = babel;
+
+  const fixContentProp = args => {


What is the expected value of args?

Can we add a bit of documentation here so it's easier for people to understand? I'd like this project to be a bit educational as well: it helped you, so it ought to help others interested in AST transformation.

Yes you're, right, I'll do a more descriptive pass soon :)

TejasQ · 2018-08-03T22:21:04Z

index.js

+    const newAttrs = attrs.filter(attr => {
+      const {value, name, type} = attr;
+      if (type === "JSXSpreadAttribute") return true;
+      if (name.name === "css") {


I think we can do better than name.name here in order to allow developers not as experienced with this stuff to understand what's happening and more clearly describe our intent.

I didn't come up with attr.name.name. That's part of the AST API.

I know that, but I'm suggesting aliasing the destructured name on line 51 in order to provide an identifier that is a little more understandable.

TejasQ · 2018-08-03T22:22:29Z

index.js

-                );
+        appendToCss.push({
+          name: t.identifier(name.name),
+          value: value.type === "JSXExpressionContainer" ? value.expression : value,


Would the t.isJSXExpressionContainer function work for this validation instead? I think it's more future proof because it could handle more imperative validations if needed in the future.

This probably applies to type === "XXX" checks, of which I did quite a lot.

TejasQ · 2018-08-03T22:25:58Z

index.js

+    if (appendToCss.length > 0) {
+      if (!cssAttr) {
+        cssAttr = t.jsxAttribute(
+          t.jsxIdentifier("css"),


Afaik, <span css={} is invalid in intrinsic JSX elements. The valid alternative is <span style={}.

Checked out this part of the emotion docs!

<span css={styles}/> will be processed by the emotion babel plugin and turned into <span className={...}/>

🤔

Yes, but is it safe to assume that everyone has the babel plugin? I'd rather the codemod give users something that works with base emotion out of the box.

We could even add a plugin option for users to opt-in to babel-plugin-specific syntax sugar like this. I know @kentcdodds wanted this and I agree that this would be a really nice feature to add to this codemod.

So what would you suggest for code like:

<glamorous.Span css={{":hover": {color: "red"}}}/>

I guess we could do something like:

// opts.withPlugin === true <span css={{":hover": {color: "red"}}}/> // opts.withPlugin === false import {css} from emotion <span className={css({":hover": {color: "red"}})}/>

But for the sake of keeping this PR small, I'd suggest to assume that the plugin is installed for now. We can introduce the withPlugin flag in a separate PR

Great suggestion.

Philosophically, for keeping the PR small, I'd remove the functionality around CSS props and other plugin-related transformations from this PR altogether and include them all in a more encapsulated PR.

This means that the existing transformations around this functionality ought not be removed, but moved to another PR so this one is a little leaner and simpler, even to review and collaborate on.

TejasQ · 2018-08-03T22:32:11Z

index.js

+        );
+        newAttrs.push(cssAttr);
+      } else if (cssAttr.value.expression.type !== "ObjectExpression") {
+        // turn <span css={obj} .../> into <span css={{...obj}} .../>


I think the intent here is to turn <span css={obj()} into <span css={{ ...obj() }}? The current description of spreading an object into an object seems to be a bit of a no-op. 😅

This code is needed to transform e.g.

<gl.Span css={redStyle} marginTop={5}/>

into

<span css={{...redStyle, marginTop: 5}}/>

So the comment doesn't tell the whole story yet, I'll fix it!

danielberndt · 2018-08-03T23:38:16Z

Also, I've noticed that changing the ImportDefaultSpecifier in the AST explorer outright removes the default import if it is not gl. 🤔

Ah, have a look at this test to understand why I did this.

Previously a reference to glamorous was needed for <glamorous.div/>, but after the transform it's only <div/>. So we can remove the import statement, if nothing else refers to glamorous.

One side effect is that if glamorous was imported and never used, the import will disappear due to the codemod. That's something I'd be fine with.

danielberndt · 2018-08-04T22:47:51Z

Okay, I just added the discussed improvements. (and added eslint)

If you're happy with the changes, I'd suggest to work on all the additional suggestions above (and in the code) in separate PRs :)

TejasQ · 2018-08-05T00:49:44Z

Thanks for the documentation adjustments and destructured aliases! Would you agree with me that it is a far more welcoming to readers of the source code now? I certainly think so. Great job!

I'll have another read through it tomorrow and possibly approve and merge it if we get the separation of PR concerns right.

One extra nit I found was that we're writing multiline comments with // on many lines. I've seen this pattern around the web and I have to say I don't get it. I much prefer using the syntax we already have for multiline comments. But, as I said, it's just a nit and not at all pivotal to the way this tool works or even reads.

It has been a real pleasure collaborating with you on this so far. 😄

danielberndt · 2018-08-05T21:43:24Z

Re separating the css property stuff into a separate PR:
fine by me, but I'll add a warning to the user that the codemod is potentially outputting broken code if it encounters a <glamorous.Div> component.
I also guess since a separate PR will be more focussed, so turnaround time will probably be quicker. (I still have two projects that I'd like to transform at some point soon)

Regarding multiline comments. To me it's mostly a matter of organical growth. Most comments start off as a single line comment, then I add a half-sentence, it gets too long, so I break it into two lines without changing the type of comment. It's a mixture of lazyness and lack of IDE-features I guess.

TejasQ · 2018-08-06T07:40:46Z

index.js

-                        )
-                    );
-                }
+  const styledVisitor = {


Can we document this a little better?

name, oldName, etc. seem a little ambiguous.

TejasQ · 2018-08-06T07:42:02Z

package.json

@@ -5,5 +5,27 @@
  "main": "index.js",
  "repository": "https://github.com/tejasq/babel-plugin-glamorous-to-emotion",
  "author": "Tejas Kumar <tejas@tejas.qa>",
-  "license": "MIT"
+  "license": "MIT",


You should add a "collaborators" here since we're buddies now! 😄

TejasQ · 2018-08-06T07:42:38Z

Great stuff! Let's add a little more explicit naming and docs and then we're good to merge. 🚀 🔥

danielberndt · 2018-08-06T13:53:46Z

Alright, I just added some clarifying comments for the core part of the transform step :)

TejasQ

This is awesome. Readable, tested, wonderful. Thanks for your contribution!

TejasQ · 2018-08-06T13:58:05Z

index.js

-                        ]),
+        // only if the traversal below wants to know the newName,
+        // we're gonna add the default import
+        const getNewName = () => {


How does this work on line 61 if it's defined here? 🤔 Surely I've missed something.

Whatever you put as a second argument when calling a traverse step, will be available as state in the second parameter of each Visitor.

I pass the getNewName parameter here

Check this part of kentcdodds best practices for more information!

Ah! I missed the part where it was being passed in on line 44! Makes sense!

danielberndt · 2018-08-06T14:10:49Z

Ah sorry, didn't expect your review to be that quick, I just made a small change to how the "contributors" field is set up. And... this seems to have dismissed your review.

TejasQ · 2018-08-06T14:32:10Z

index.js

-                        ]),
+        // only if the traversal below wants to know the newName,
+        // we're gonna add the default import
+        const getNewName = () => {


Ah! I missed the part where it was being passed in on line 44! Makes sense!

danielberndt added 2 commits August 3, 2018 17:27

Rewrite

04bce41

fix undefined variable

a2ded59

TejasQ requested changes Aug 3, 2018

View reviewed changes

danielberndt added 2 commits August 4, 2018 23:34

Readability fixes for code

c29a761

adding eslint

e887472

remove jsx transforms

5a1d3a1

TejasQ requested changes Aug 6, 2018

View reviewed changes

TejasQ mentioned this pull request Aug 6, 2018

JSX transforms #3

Merged

3 tasks

TejasQ previously approved these changes Aug 6, 2018

View reviewed changes

TejasQ reviewed Aug 6, 2018

View reviewed changes

danielberndt added 2 commits August 6, 2018 16:00

Add Daniel Berndt as "contributor"

fed5d1b

Improve documentation for glamorousVisitor

127e82f

danielberndt dismissed TejasQ’s stale review via 127e82f August 6, 2018 14:00

danielberndt force-pushed the master branch from f63cb44 to 127e82f Compare August 6, 2018 14:00

TejasQ approved these changes Aug 6, 2018

View reviewed changes

TejasQ merged commit d68ecbc into TejasQ:master Aug 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite of the codemod #2

Rewrite of the codemod #2

danielberndt commented Aug 3, 2018

danielberndt commented Aug 3, 2018

TejasQ commented Aug 3, 2018

danielberndt commented Aug 3, 2018

TejasQ commented Aug 3, 2018

TejasQ left a comment

TejasQ Aug 3, 2018

danielberndt Aug 3, 2018

TejasQ Aug 3, 2018

danielberndt Aug 3, 2018

TejasQ Aug 4, 2018

TejasQ Aug 3, 2018

danielberndt Aug 3, 2018

TejasQ Aug 3, 2018

danielberndt Aug 3, 2018

TejasQ Aug 4, 2018

danielberndt Aug 4, 2018

TejasQ Aug 5, 2018 •

edited

Loading

TejasQ Aug 3, 2018

danielberndt Aug 3, 2018

danielberndt commented Aug 3, 2018

danielberndt commented Aug 4, 2018

TejasQ commented Aug 5, 2018

danielberndt commented Aug 5, 2018

TejasQ Aug 6, 2018

TejasQ Aug 6, 2018

TejasQ commented Aug 6, 2018

danielberndt commented Aug 6, 2018

TejasQ left a comment

TejasQ Aug 6, 2018

danielberndt Aug 6, 2018

TejasQ Aug 6, 2018

danielberndt commented Aug 6, 2018

TejasQ Aug 6, 2018

Rewrite of the codemod #2

Rewrite of the codemod #2

Conversation

danielberndt commented Aug 3, 2018

danielberndt commented Aug 3, 2018

TejasQ commented Aug 3, 2018

danielberndt commented Aug 3, 2018

TejasQ commented Aug 3, 2018

TejasQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejasQ Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielberndt commented Aug 3, 2018

danielberndt commented Aug 4, 2018

TejasQ commented Aug 5, 2018

danielberndt commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejasQ commented Aug 6, 2018

danielberndt commented Aug 6, 2018

TejasQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielberndt commented Aug 6, 2018

Choose a reason for hiding this comment

TejasQ Aug 5, 2018 •

edited

Loading