-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
fix auto import error with whitespace JSXText #11354
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
Conversation
!(t.isJSXText(child) && child.value.trim() === ""), | ||
); | ||
|
||
const validChildren = openingPath.parent.children.filter(child => { |
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.
Can we reuse t.react.buildChildren
?
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.
t.react.buildChildren
mutates the node. I can refactor it to return a value instead of mutating if that's okay with you.
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.
I can refactor it to return a value instead of mutating if that's okay with you.
I am okay with that if it turns out we does not depend on its mutation behaviors. BTW t.react.buildChildren
is undocumented so it is likely only used by our plugins.
Since t.react.buildChildren
is also scattered in this plugin. It will be of great help to maintainers if we can preserve consistency or we clearly identify the different scenarios.
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.
Ah actually maybe you're right. Let me update this diff
873b2e2
to
6a2bcbb
Compare
|
||
class MobileHomeActivityTaskPriorityIcon extends React.PureComponent { | ||
render() { | ||
return <Text> {this.props.value}nbsp;</Text>; |
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.
Is this supposed to be
?
return <Text> {this.props.value}nbsp;</Text>; | |
return <Text> {this.props.value} </Text>; |
|
||
class MobileHomeActivityTaskPriorityIcon extends React.PureComponent { | ||
render() { | ||
return <Text> {this.props.value}nbsp;</Text>; |
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.
Ditto.
return <Text> {this.props.value}nbsp;</Text>; | |
return <Text> {this.props.value} </Text>; |
6a2bcbb
to
52eee29
Compare
The new JSX auto import React has a bug where it ignores valid whitespace text when calculating whether to import
jsx
orjsxs
. This fixes it by stripping JSXText the same way thatt.react.buildChildren
strips JSXText so that the children calculated are consistent with each other.