Skip to content

Fix #5046: Adjacent JSX #5049

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

Merged
merged 5 commits into from
May 20, 2018
Merged

Fix #5046: Adjacent JSX #5049

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
ba01407
Fix #5046: Adjacent JSX
zdenko May 2, 2018
09465f7
check CSX only when wrapped in parentheses
zdenko May 2, 2018
e33f032
Fix indentation
GeoffreyBooth May 19, 2018
89405a1
Add test for unlikely, but valid, JSX syntax
GeoffreyBooth May 19, 2018
22cd4e3
Merge branch 'master' into jsx
GeoffreyBooth May 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions lib/coffeescript/nodes.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 11 additions & 6 deletions src/nodes.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,16 +478,21 @@ exports.Block = class Block extends Base
# ensures that the final expression is returned.
makeReturn: (res) ->
len = @expressions.length
[..., lastExp] = @expressions
lastExp = lastExp?.unwrap() or no
# We also need to check that we’re not returning a CSX tag if there’s an
# adjacent one at the same level; JSX doesn’t allow that.
if lastExp and lastExp instanceof Parens and lastExp.body.expressions.length > 1
{body:{expressions}} = lastExp
[..., penult, last] = expressions
penult = penult.unwrap()
last = last.unwrap()
if penult instanceof Call and penult.csx and last instanceof Call and last.csx
expressions[expressions.length - 1].error 'Adjacent JSX elements must be wrapped in an enclosing tag'
while len--
expr = @expressions[len]
@expressions[len] = expr.makeReturn res
@expressions.splice(len, 1) if expr instanceof Return and not expr.expression
# We also need to check that we’re not returning a CSX tag if there’s an
# adjacent one at the same level; JSX doesn’t allow that.
if expr.unwrapAll().csx
for csxCheckIndex in [len..0]
if @expressions[csxCheckIndex].unwrapAll().csx
expr.error 'Adjacent JSX elements must be wrapped in an enclosing tag'
break
this

Expand Down
27 changes: 22 additions & 5 deletions test/csx.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,11 +771,11 @@ test 'JSX fragments: fragment with text nodes', ->

test 'JSX fragments: fragment with component nodes', ->
eqJS '''
Component = (props) =>
<Fragment>
<OtherComponent />
<OtherComponent />
</Fragment>
Component = (props) =>
<Fragment>
<OtherComponent />
<OtherComponent />
</Fragment>
''', '''
var Component;

Expand Down Expand Up @@ -821,3 +821,20 @@ test '#5055: JSX expression indentation bug', ->
{typeof a !== "undefined" && a !== null ? a : <span />}
</div>;
'''

# JSX is like XML, in that there needs to be a root element; but
# technically, adjacent top-level elements where only the last one
# is returned (as opposed to a fragment or root element) is permissible
# syntax. It’s almost certainly an error, but it’s valid, so need to leave it
# to linters to catch. https://github.com/jashkenas/coffeescript/pull/5049
test '“Adjacent” tags on separate lines should still compile', ->
eqJS '''
->
<a />
<b />
''', '''
(function() {
<a />;
return <b />;
});
'''
7 changes: 4 additions & 3 deletions test/error_messages.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1664,25 +1664,26 @@ test 'CSX error: invalid attributes', ->

test '#5034: CSX error: Adjacent JSX elements must be wrapped in an enclosing tag', ->
assertErrorFormat '''
render = ->
render = -> (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should test that it errors out both with and without parens

Copy link
Collaborator

Choose a reason for hiding this comment

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

But the without-parens version is valid JSX:

->
  <a />
  <b />
(function() {
  <a />;
  return <b />;
});
(function () {
  React.createElement("a", null);
  return React.createElement("b", null);
});

I agree it’s likely a mistake on the user’s part, but catching “most of the time this is wrong” errors is the job of a linter, not the compiler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I talked about this a bit earlier.
You could say that, but you could also say that

->
  <a />
  <b />

really is

->
  return (
    <a />
    <b />
  )

just like

->
  a: 1
  b: 2

is

->
  return {
    a: 1
    b: 2
  }

instead of

->
  {a: 1}
  return {b: 2}

Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth May 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don’t think you could say that parentheses are implied. JSX tags on subsequent lines aren’t assumed to be part of a unified construct the way an object is. They don’t compile into a single entity, for one thing, even with parentheses:

jsx = (
  <a />
  <b />
)
jsx = (<a />, <b />);

That’s not the way objects behave. The separate keys of an object are unified by virtue of being all part of the same block, as defined by indentation. Adjacent JSX tags are just that, adjacent JSX tags, unless they’re enclosed by a parent tag or fragment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. I also want to point out that this:

I agree it’s likely a mistake on the user’s part, but catching “most of the time this is wrong” errors is the job of a linter, not the compiler.

Could very well apply to the original warning. Babel could compile it to two createElement doing nothing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could very well apply to the original warning. Babel could compile it to two createElement doing nothing.

It could, but the JSX spec tells it not to. I think that's the distinction.

<Row>a</Row>
<Row>b</Row>
)
''', '''
[stdin]:3:4: error: Adjacent JSX elements must be wrapped in an enclosing tag
<Row>b</Row>
^^^^^^^^^^^
'''
assertErrorFormat '''
render = -> (
a = "foo"
<Row>a</Row>
<Row>b</Row>
)
''', '''
[stdin]:3:4: error: Adjacent JSX elements must be wrapped in an enclosing tag
[stdin]:4:4: error: Adjacent JSX elements must be wrapped in an enclosing tag
<Row>b</Row>
^^^^^^^^^^^
Copy link
Collaborator

Choose a reason for hiding this comment

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

@zdenko We should also add a test for the non-error case:

->
  <a />
  <b />

That would be a good place to put a comment with a link to this PR, and an explanation for why this shouldn’t error.

'''

test 'Bound method called as callback before binding throws runtime error', ->
class Base
constructor: ->
Expand Down