-
Notifications
You must be signed in to change notification settings - Fork 290
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
Add codemod for 'React.DOM.div' -> 'React.createElement("div"' #133
Conversation
specifier.imported && | ||
specifier.imported.name === DOMModuleName | ||
); | ||
const removeDeconstructedDOMStatement = (j, root) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a more accurate name would be 'replaceDeconstructedDOMStatement'.
In each case it replaces it with 'createElement'.
hasModifications = | ||
removeDeconstructedDOMStatement(j, root) || hasModifications; | ||
|
||
// is 'DOM' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment probably was not needed
j(DOMFactoryPath).replaceWith( | ||
j.identifier('createElement') // TODO; fix | ||
); | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops - removing this.
} | ||
} | ||
|
||
ReactDOM.render( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably remove this part in each example.
import {DOM} from 'Free'; | ||
|
||
const foo = DOM.div('a', 'b', 'c'); | ||
const foo = Free.DOM.div('a', 'b', 'c'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is just an example, we could use 'bar' instead of 'foo' for the second const.
Since we are deprecating the 'React.DOM.*' factories,[1] we want to provide a codemod so that it's easier for folks to upgrade their code. This will include an option to use 'React.createFactory' instead of 'React.createElement' in case there is a use case where that is preferred. There is one use of `React.DOM.*` that I have seen which is not covered here - sometimes it has mistakenly been used for Flow typing. In the cases I have found it is not proper syntax and doesn't seem like something we should cover with this codemod. [1]: facebook/react#9398 and facebook/react#8356
18061dd
to
26f661f
Compare
|
||
const isDOMIdentifier = path => ( | ||
path.node.name === DOMModuleName && | ||
path.parent.parent.node.type === 'CallExpression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will match any function called DOM
. Could we restrict this so it only matches if there's a corresponding import/require statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you are thinking of that!
The way I structured the codemod, it only gets used after we have already detected the corresponding import/require statement. I can restructure things to make that more clear.
That's also why I added this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh great, didn't see that!
We want to make sure that only references to `{DOM} = React;` are transformed, and not `{DOM} = Foobar;` or anything else. This makes the code a bit more clear in that intention.
Since we are deprecating the 'React.DOM.*' factories,(facebook/react#9398 and facebook/react#8356) we want to
provide a codemod so that it's easier for folks to upgrade their code.
This will include an option to use 'React.createFactory' instead of
'React.createElement' in case there is a use case where that is
preferred.
There is one use of
React.DOM.*
that I have seen which is not coveredhere - sometimes it has mistakenly been used for Flow typing. In the
cases I have found it is not proper syntax and doesn't seem like
something we should cover with this codemod.