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.

Sign up for GitHub

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

Jump to bottom

Several JSX fixes (addresses #73) #123

Merged
merged 5 commits into from Jan 13, 2017
Merged

Several JSX fixes (addresses #73) #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22aef7c
Wrap Stateless JSX Arrow Functions and Assignment in Parens (fixes pa…
rattrayalex Jan 11, 2017
85a07d4
Break JSXOpeningElement between attributes (fixes #15)
rattrayalex Jan 11, 2017
2552da5
JSX maintains spaces that matter (fixes #30 and thus part of #73)
rattrayalex Jan 11, 2017
5075fa6
Multiline JSX opening tag breaks children out too (for #73)
rattrayalex Jan 12, 2017
eccf3d1
Add regression tests for long JSX Expression contents
rattrayalex Jan 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
158 changes: 91 additions & 67 deletions src/printer.js
View file
Open in desktop
Expand Up @@ -343,12 +343,20 @@ function genericPrintNoParens(path, options, print) {

parts.push(" =>");

const body = path.call(print, "body");
const collapsed = concat([ concat(parts), " ", body ]);

if (n.body.type === "JSXElement") {
return group(collapsed);
}

return conditionalGroup([
concat([ concat(parts), " ", path.call(print, "body") ]),
concat([ concat(parts),
indent(options.tabWidth,
concat([line, path.call(print, "body")]))])
])
collapsed,
concat([
concat(parts),
indent(options.tabWidth, concat([ line, body ])),
]),
]);
case "MethodDefinition":
if (n.static) {
parts.push("static ");
Expand Down Expand Up @@ -545,22 +553,8 @@ function genericPrintNoParens(path, options, print) {
case "ReturnStatement":
parts.push("return");

var arg = path.call(print, "argument");

if (n.argument) {
if (
namedTypes.JSXElement && namedTypes.JSXElement.check(n.argument) &&
hasHardLine(arg)
) {
parts.push(
" (",
indent(options.tabWidth, concat([ hardline, arg ])),
hardline,
")"
);
} else {
parts.push(" ", arg);
}
parts.push(" ", path.call(print, "argument"));
}

parts.push(";");
Expand Down Expand Up @@ -1075,57 +1069,19 @@ function genericPrintNoParens(path, options, print) {
"}"
]));
case "JSXElement":
var openingLines = path.call(print, "openingElement");

if (n.openingElement.selfClosing) {
assert.ok(!n.closingElement);

return openingLines;
}

var children = [];

path.map(
function(childPath) {
var child = childPath.getValue();

if (
namedTypes.Literal.check(child) && typeof child.value === "string"
) {
if (/\S/.test(child.value)) {
const beginBreak = child.value.match(/^\s*\n/);
const endBreak = child.value.match(/\n\s*$/);

children.push(
beginBreak ? hardline : "",
child.value.replace(/^\s+|\s+$/g, ""),
endBreak ? hardline : ""
);
} else if (/\n/.test(child.value)) {
children.push(hardline);
}
} else {
children.push(print(childPath));
}
},
"children"
);

var mostChildren = children.slice(0, -1);
var closingLines = path.call(print, "closingElement");
return concat([
openingLines,
indent(options.tabWidth, concat(mostChildren)),
util.getLast(children) || "",
closingLines
]);
return printJSXElement(path, options, print);
case "JSXOpeningElement":
return group(
concat([
"<",
path.call(print, "name"),
concat(path.map(attr => concat([" ", print(attr)]), "attributes")),
n.selfClosing ? " />" : ">"
multilineGroup(concat([
indent(options.tabWidth,
concat(path.map(attr => concat([line, print(attr)]), "attributes"))
),
n.selfClosing ? line : softline,
])),
n.selfClosing ? "/>" : ">"
])
);
case "JSXClosingElement":
Expand Down Expand Up @@ -1184,7 +1140,7 @@ function genericPrintNoParens(path, options, print) {
} else if (n.variance === "minus") {
key = concat([ "-", key ]);
}
}
}

parts.push(key);

Expand Down Expand Up @@ -2036,6 +1992,74 @@ function printMemberChain(node, options, print) {
}
}

function printJSXElement(path, options, print) {
const n = path.getValue();
var openingLines = path.call(print, "openingElement");

let elem;
if (n.openingElement.selfClosing) {
assert.ok(!n.closingElement);

elem = openingLines;
} else {
var children = [];

path.map(
function(childPath) {
var child = childPath.getValue();

if (
namedTypes.Literal.check(child) && typeof child.value === "string"
) {
if (/\S/.test(child.value)) {
const beginBreak = child.value.match(/^\s*\n/);
const endBreak = child.value.match(/\n\s*$/);

children.push(
beginBreak ? hardline : "",
child.value.replace(/^\s+|\s+$/g, endBreak ? "" : " "),
endBreak ? hardline : ""
);
} else if (/\n/.test(child.value)) {
children.push(hardline);
}
} else {
children.push(print(childPath));
}
},
"children"
);

var mostChildren = children.slice(0, -1);
var closingLines = path.call(print, "closingElement");

const firstChild = children[0];
const lastChild = util.getLast(children);
const beginBreak = firstChild && firstChild.type === "line";
const endBreak = lastChild && lastChild.type === "line";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlongster my beginBreak / endBreak logic here felt a bit awkward – I'm adding a softline unless there's already a line. Is there a cleaner way to do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rattrayalex Sorry I didn't review this until now.

I'm not entirely sure of the logic you're going for here, but is util.newlineExistsBefore helpful? If you're trying to emulate the breaking behavior in the original source, that and util.newlineExistsAfter allows you to peek into the original source and see if a node is on a newline or not. Here's how you use it:

util.newlineExistsBefore(text, util.locStart(node))

What this does is it scans backward until it hits either a newline or a non-whitespace character. If it hits a newline, it reports true, otherwise it's false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely helpful to know about, though in this case I think the lines could be inserted by prettier, not just from the original source. So I think it wouldn't be adequate in this case.

But it would be very helpful when deciding whether {' '} needs to be inserted, if that's a route we go.


elem = multilineGroup(concat([
openingLines,
indent(options.tabWidth, concat([ beginBreak ? "" : softline ].concat(mostChildren))),
lastChild || "",
endBreak ? "" : softline,
closingLines,
]));
}

const parent = path.getParentNode();
if (!parent || parent.type === "JSXElement" || parent.type === "ExpressionStatement") {
return elem;
}

return multilineGroup(concat([
ifBreak("("),
indent(options.tabWidth, concat([ softline, elem ])),
softline,
ifBreak(")"),
]));
}

function adjustClause(clause, options, forceSpace) {
if (isCurlyBracket(clause) || forceSpace) {
return concat([ " ", clause ]);
Expand Down
6 changes: 3 additions & 3 deletions tests/jsx_intrinsics.builtin/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Expand Up @@ -39,9 +39,9 @@ var c: React.Element<any> = <div not_a_real_attr=\"asdf\" />;
// different attributes.
var d: React.Element<{ doesntmatch: string }> = <div not_a_real_attr=\"asdf\" />;
// No error as long as expectations are consistent, though.
var e: React.Element<{
not_a_real_attr: string
}> = <div not_a_real_attr=\"asdf\" />;
var e: React.Element<{ not_a_real_attr: string }> = (
<div not_a_real_attr=\"asdf\" />
);
"
`;

Expand Down
6 changes: 3 additions & 3 deletions tests/jsx_intrinsics.custom/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Expand Up @@ -32,9 +32,9 @@ var b: React.Element<{ prop1: string }> = <CustomComponent prop=\"asdf\" />;
<div id={42} />;
// Error: (\`id\` prop) number ~> string
var c: React.Element<{ id: string }> = <div id=\"asdf\" />;
var d: React.Element<{
id: number
}> = <div id=\"asdf\" />; // Error: Props<{id:string}> ~> Props<{id:number}>
var d: React.Element<{ id: number }> = (
<div id=\"asdf\" />
); // Error: Props<{id:string}> ~> Props<{id:number}>
"
`;

Expand Down