Feat - Adds basic code for implementing migrate command #28
Conversation
|
I'll test it locally later when I got time. Good job 👍 |
|
|
||
| if(!module) { | ||
| module = j.property('init', j.identifier('modules'), j.arrayExpression(modulesVal)); | ||
| //aert(JSON.stringify(p.node.value.properties)) |
There was a problem hiding this comment.
Fix this typo. could actually be better with a brief explaination instead. 😺
| @@ -0,0 +1,46 @@ | |||
| // Press ctrl+space for code completion | |||
| export default function transformer(file, api) { | |||
There was a problem hiding this comment.
I am not yet so much deep into de code and the logic, and is quite hard to understand what is this pr about, imho, in order to make the project more approachable for contributors, we should right away add documentation to the functions that we are doing and add some unit tests that help the reviewers who have no context on a PR to understand what they are reviewing
There was a problem hiding this comment.
Hi @ev1stensberg one question about this:
We should use recast instead of jscodeshift eventually.
What would be the advantage of using recast over jscodesnift ?
There was a problem hiding this comment.
@DanielaValero - This is a WIP for me. Lets setup some working stuff before we open this to contributors. I'm hesitant to give a timeline for this as I can't commit fixed time everyday. But I have a plan to explore a couple more of these transformers and get it working. Then, I ll be able to abstract out common scenarios by writing utils. Then I will put all of these transformers under webpack --migrate at which point we can open this up to future contributors. I will update all of this in #29 and #6 . For speed and brevity, I prefer putting up PRs of whatever work I complete as long as it does not break existing functionality.
There was a problem hiding this comment.
Hi @pksjce, totally agree with that, my main concern at the moment is being able to understand what you are doing, so I can actually do code reviews and pick up some work too. At the moment, without context and understanding the ideas behind it, what the code is gonna do, etc, I am not able to do any of both. Thanks for the pointer you shared below!
There was a problem hiding this comment.
Yeah. That is a concern. I've better understood this now. So I will update the issue with better info by the end of this week!
Also lets Dm on slack if you have any specific questions 👍
| // Press ctrl+space for code completion | ||
| export default function transformer(file, api) { | ||
| const j = api.jscodeshift; | ||
|
|
There was a problem hiding this comment.
In general, I am more a fan of using whole words instead of letters or abbreviations for variables and/or functions, it helps new developers to have a better understanding on the code they are reading
There was a problem hiding this comment.
Also for context on what I'm doing here, refer to other codemods like jest-codemods
|
Squash your commits and it's all good on my end |
pksjce
force-pushed
the
feat/config-migration
branch
from
4db513b
to
9333b41
Compare
pksjce
changed the title
pksjce
force-pushed
the
feat/config-migration
branch
2 times, most recently
from
863c916
to
b3e33e8
Compare
pksjce
force-pushed
the
feat/config-migration
branch
from
b3e33e8
to
5ffbe81
Compare
|
@ev1stensberg @DanielaValero Basically tried implementing a raw version of what I described in #6. |
|
@pksjce Do you want this merged now or? Squashing not needed, multiple commits across files |
|
Sure :) Will send more fixes and improvements in new PRs |
What kind of change does this PR introduce?
Adds a raw implementation of migrate.
Adds transformation for resolve.root
Here's a gif of the change
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
No
Other information