[CS2] CSX spread attributes: <div {props…} /> #4607

zdenko · 2017-07-08T12:46:07Z

This PR addresses the issue with CSX spread attributes (#4551).

Since spread in JSX is special notation different from the ES6 generic spread syntax, CS will simply compile {props...} to {...props}.

# <div {props...}>hello</div>
<div {...props}>hello</div>

# <div {props1...} id="tagId" {props2...}>hello</div>
<div {...props1} id="tagId" {...props2}>hello</div>

There is also an "accidental" side effect and this:

<div {props..., a: "x"}>hello</div>
<div {props1..., props2...}>hello</div>

will compile to

<div {...props} a="x">hello</div>
<div {...props1} {...props2}>hello</div>

GeoffreyBooth · 2017-07-09T05:37:03Z

I wonder if we should be doing this checking in the lexer . . . shouldn’t the check if a previous interpolation is a spread be happening in nodes.coffee?

Other than that it looks good to me. @lydell or @connec?

zdenko · 2017-07-09T07:02:43Z

shouldn’t the check if a previous interpolation is a spread be happening in nodes.coffee?

@GeoffreyBooth I don't think this is currently possible because of #4600.

Lexer (originally) converts <div id="bar" {foo...} /> into
[CSX_TAG div] [CALL_START (] [{ {] [PROPERTY id] [: :] [STRING "bar"] [, ,] [( (] [{ {] [IDENTIFIER foo] [... ...] [} }] [) )] [, ,] [} }] [CALL_END )].

This PR converts the same code into
[CSX_TAG div] [CALL_START (] [{ {] [PROPERTY id] [: :] [STRING "bar"] [, ,] [IDENTIFIER foo] [... ...] [, ,] [} }] [CALL_END )].

xixixao · 2017-07-09T11:32:11Z

This is indeed what I had in mind in terms of what it would compile to. Nice! No opinion on the implementation.

GeoffreyBooth · 2017-07-09T17:41:57Z

@zdenko Those tokens roll up into the Call node, where you have compileCSX that works with attributes and content. Is it not possible to look inside those arrays to check for duplicates there?

zdenko · 2017-07-09T18:43:13Z

@GeoffreyBooth I've tried, but compilation fails at parsing step (before the Call).
Since @xixixao's lexer method csxToken is already transforming CSX tokens, I just added a simple check for spread attribute and alter the transformation.

But, I'm also working on the PR for #4600 and it might be possible to move this into the compileCSX if necessary.

GeoffreyBooth · 2017-07-09T20:33:00Z

In general we try to do as much as possible in Nodes rather than in Lexer, because checks in Lexer are much more brittle and harder to maintain. If that’s not possible that’s fine.

Should we merge this in now or wait until your other PR is ready?

zdenko · 2017-07-09T22:50:30Z

I would merge now. I'm not sure when the PR for #4600 will be ready.

GeoffreyBooth · 2017-07-09T23:59:12Z

Okay. @lydell and @connec, any notes?

connec · 2017-07-10T08:59:06Z

I guess syntactically it's a bit weird that <div {a: 1}> compiles but <div {object}> or <div {object = a: 1}> won't. Unfortunately my lexer-fu is weak, so I'm not sure if there's a clean way of blocking a: 1 whilst still allowing splats?

Edit: in fact there are a couple of edge cases I've found whilst playing around:

<div {obj}></div> compiles to <div obj></div>
<div {"#{a}": 1}></div> compiles to the invalid <div [${a}]=1></div>
<div {"#{a}"}></div> throws an error: TypeError: Cannot set property 'csxAttribute' of undefined - here.

I think it would be best if we could restrict the syntax to allowing a single spread expression only, as is the case with JSX.

zdenko · 2017-07-12T00:27:34Z

I added some validation for CSX attributes.
Valid CSX syntax:

<div id="value" name={someName} {props...} attr="#{attribute}" checked />

Since CSX attributes are transformed into Obj in the lexer (@xixixao might explain this better), the following CS syntax is also allowed and will produce the same result.

<div {id:"value"} {name:{someName}} {props...} {attr:"#{attribute}"} {checked} />

zdenko · 2017-07-19T19:23:44Z

@GeoffreyBooth I think my work is done here. Do I have to add anything else?

GeoffreyBooth · 2017-07-19T21:41:21Z

Assuming this now covers all the cases @connec mentioned, then hopefully this is ready. @connec, did you have any other notes?

GeoffreyBooth · 2017-07-23T02:33:34Z

@connec, any notes on this?

connec · 2017-07-23T10:36:54Z

src/nodes.coffee

+      if @csx and prop instanceof Assign
+        prop.variable.error "Unexpected token" unless prop.variable.unwrap() instanceof PropertyName
+        propVal = prop.value.unwrap()
+        unless ((propVal instanceof Parens or propVal instanceof StringWithInterpolations) and propVal.body instanceof Block) or propVal instanceof StringLiteral or propVal instanceof Obj


Could we reformat this line to something like:

unless propVal instanceof StringLiteral or propVal instanceof Obj or ((propVal instanceof Parens or propVal instanceof StringWithInterpolations) and propVal.body instanceof Block) prop.value.error "..."

It might also benefit from a comment stating what is allowed (e.g. what is (propVal instanceof Parens or propVal instanceof StringWithInterpolations) and propVal.body instanceof Block ensuring?).

connec · 2017-07-24T10:25:43Z

Thanks for the update @zdenko, that makes sense to me now!

Unfortunately, there are still a couple of 'regressions' in terms of what will compile now and the errors:

Given:
```
<div {obj}></div>
```
Used to throw:
```
[stdin]:1:10: error: unexpected ,
<div {obj}></div>
         ^
```
Now compiles to:
```
<div obj></div>;
```
This definitely shouldn't compile. The {} in the source implies that obj is a variable name, but it is instead compiled to a boolean flag.

Given:

<div {"#{a}"}></div>

Used to throw:

[stdin]:1:13: error: unexpected ,
<div {"#{a}"}></div>
            ^

Now throws:

SyntaxError: Unexpected token

For some reason this error has lost location data.

Given:
```
<div {props..., a: "x"}>hello</div>
<div {props1..., props2...}>hello</div>
```
As you note in the PR description, these now compile to:
```
<div {...props} a="x">hello</div>
<div {...props1} {...props2}>hello</div>
```
I'm not entirely comfortable with this for a couple of reasons:
- From these examples you might infer that any object expression is valid, however <div {a: 1}></div> is invalid.
- This leads to two ways of adding attributes to a CSX expression, should <div {a..., b: "c"}> or <div {a...} b="c"> be preferred?
  I don't want to belabour this point too much though, if someone else thinks it's acceptable (@GeoffreyBooth / @lydell / @vendethiel / @xixixao).

microdou · 2017-07-24T16:50:24Z

@zdenko actually added new functionality to the original jsx spreads, by allowing the following cases:

<div {props..., a: "x"}>hello</div>
<div {props1..., props2...}>hello</div>

However, as @connec points out, the new syntax may be causing some problems, so it's debatable that whether these new syntax should be allowed in coffeescript v2.0.0.

# If this is allowed
<div {props..., a: "x"}>hello</div>

# Should this be allowed?
<div {a: "x"}>hello</div>

JSX only allows {...obj} inside the bracket, which is not a real spread at all. I think this syntax is only introduced into JSX so that an object won't be treated as a Boolean flag.

<div {...obj}>hello</div>
// transpiles to
React.createElement(
  "div",
  obj,
  "hello"
);
// obj is treated as an object

<div obj>hello</div>
// transpiles to
React.createElement(
  "div",
  { obj: true },
  "hello"
);
// obj is treated as a Boolean flag

Though ugly, the "jsx spread" obj can actually appear in a complex form and be correctly transpiled in babel.

<div {...{...obj, a: "x"}}>hello</div>
// transpiles to
React.createElement(
  "div",
  _extends({}, obj, { a: "x" }),
  "hello"
);
// a new object {...obj, a: "x"} is created and gets "jsx spread"
// babel correctly recognize the two different "..." here

I would suggest that CSX follows JSX syntax strictly and safely, by only allowing {obj...} inside < >. Nothing else should appear side-by-side withobj... inside {obj...}. Neither {obj..., a: "x"} nor {obj1..., obj2...} is valid. However, a complex form of obj can be allowed, which follows just regular coffeescript syntax, such as {{obj..., a: "x"}...}, {{obj1..., obj2...}...} and {{a: "x"}...}. {{obj1..., obj2...}...} is of course ugly, {obj1...} {obj2...} inside < > is a better form.

In all, I suggest only one object can be filled into the blank to the direct left of ..., nothing else inside {} should appear before the object or after the ...:

<div {___...}>hello</div>

zdenko · 2017-07-24T22:32:24Z

@connec, @microdou you're right. CSX syntax should follow JSX.
The main issue is that most of the CSX transformation happens inside lexer where attribute checked in <div checked /> or <div {checked} /> is transformed into Obj {checked} in both cases.
This means that most of the CSX syntax checking has to be placed inside the lexer.

zdenko · 2017-07-24T22:41:51Z

BTW this new "functionality" (e.g. <div {props..., a:1} ?>) was just side effect caused by enabling spread synatx in the lexer. It wasn't intentional.

GeoffreyBooth · 2017-07-24T22:47:18Z

src/lexer.coffee

@@ -10,7 +10,7 @@
 # are read by jison in the `parser.lexer` function defined in coffeescript.coffee.

 {Rewriter, INVERSES} = require './rewriter'
-
+log = console.log


This shouldn’t be here.

GeoffreyBooth · 2017-07-24T22:47:26Z

src/lexer.coffee

@@ -109,6 +109,20 @@ exports.Lexer = class Lexer
  # though `is` means `===` otherwise.
  identifierToken: ->
    inCSXTag = @atCSXTag()
+    # Check CSX properties synatx.


“syntax”

GeoffreyBooth · 2017-07-24T22:47:45Z

src/nodes.coffee

@@ -1207,7 +1207,7 @@ exports.Obj = class Obj extends Base
      else
        ',\n'
      indent = if isCompact or prop instanceof Comment then '' else idt
-
+      log = console.log


Please remove.

microdou · 2017-07-28T18:12:10Z

@zdenko Perfect!

microdou · 2017-07-28T20:46:52Z

I just realized that {...props} is allowed.

<div {...props}></div>
# compiles to
<div {...props}></div>;

I thought it was an side effect until I found that it appears to be universally allowed in coffeescript now.

newObj = {...obj, a: "x"}
# compiles to
var newObj;
newObj = Object.assign({}, obj, {
  a: "x"
});

@GeoffreyBooth Is the left-sided-spreads officially supported by coffeescript now in addition to the legacy right-sided-spreads?

GeoffreyBooth · 2017-07-28T20:58:25Z

Is the left-sided-spreads officially supported by coffeescript now in addition to the legacy right-sided-spreads?

Yes, since #4606. Dots on the left are not the preferred style, so I don’t plan to add it to the docs, but it seemed like a minor enough convenience to provide for people copying and pasting ES code.

connec

This looks really good now, there's only one technical issue I can see which is a bit of an edge case anyway (how many people directly use JSX expressions in attributes, I wonder?)

connec · 2017-07-31T11:05:47Z

src/lexer.coffee

@@ -500,25 +503,30 @@ exports.Lexer = class Lexer
      [input, id, colon] = match
      origin = @token 'CSX_TAG', id, 1, id.length
      @token 'CALL_START', '('
-      @token '{', '{'
+      csxStart = @token '[', '['


Unused assignment.

connec · 2017-07-31T11:16:47Z

src/lexer.coffee

-        token = @token '(', '('
+        if prevChar is ':'
+          token = @token '(', '('
+          @csxObjAttribute = no


Not sure what the right fix here is, but because this is limited to a single state, alternately nesting assignment and splats causes errors:

[stdin]:1:24: error: unexpected } <A child={<B {...params} />} /> ^

I suppose that's what @pair is for, but that will currently throw if it doesn't match... It would be nice to avoid a new stack though...

@connec the latest commit fixes this bug.

microdou · 2017-07-31T16:12:20Z

@connec @zdenko Thanks for pointing out and the fix!
Yes, it's quite useful to use JSX expression as attributes for oneliners, for example in react-bootstrap:

<ButtonToolbar>
  <OverlayTrigger overlay={<Tooltip>Hello</Tooltip>}>
    <Button>Click me!</Button>
  </OverlayTrigger>
</ButtonToolbar>

The nested JSX expression can be refactored out though:

tooltip = <Tooltip>Hello</Tooltip>
<ButtonToolbar>
  <OverlayTrigger overlay={tooltip}>
    <Button>Click me!</Button>
  </OverlayTrigger>
</ButtonToolbar>

It may be unusual to have spreads in the nested expression, but it's awesome to have the edge case covered!

GeoffreyBooth · 2017-08-01T20:19:34Z

So @microdou does the current code cover all the relevant cases, in your opinion? In other words, is it ready to merge in?

microdou · 2017-08-01T20:41:03Z

@GeoffreyBooth I couldn't find any problem. @zdenko did such an awesome job!

GeoffreyBooth · 2017-08-01T22:12:38Z

@connec, any more notes?

GeoffreyBooth · 2017-08-02T07:37:45Z

src/nodes.coffee

+        if not (attr instanceof Obj or attr instanceof IdentifierLiteral) or (attr instanceof Obj and not attr.generated and (attrProps.length > 1 or not (attrProps[0] instanceof Splat)))
+          obj.error """
+            Unexpected token. Allowed CSX attributes are: id="val", src={source}, {props...} or attribute.
+            Example: <div id="val" src={getTheSource()} {props...} checked>hello world</div>.


We don’t normally put examples in error messages. Please update the docs with this if the docs are insufficient.

I'll remove it.

GeoffreyBooth · 2017-08-02T07:39:47Z

test/csx.coffee

@@ -724,3 +692,11 @@ test 'unspaced less than after CSX works but is not encouraged', ->

      res = 2 < div;
    '''
+
+test 'CSX invalid attribute errors', ->


All our error-checking tests are in error_messages.coffee. Perhaps they shouldn’t be, but that’s a refactor for another PR. Please move these checks over there and follow the form in that file, where you confirm that the error returned is as expected (i.e. not just that an exception is thrown).

Agree. I'll move and improve the tests later today.

GeoffreyBooth · 2017-08-02T07:40:30Z

My notes are fairly minor. This is much cleaner than before, thanks for reworking it @zdenko!

connec · 2017-08-02T15:46:36Z

I'm now seeing the following issue, but it's getting very esoteric now:

<A {...{child: <B {...params} />}} />
# [stdin]:1:34: error: unexpected )
# <A {...{child: <B {...params} />}} />
#                                  ^

The same thing compiles with babel and also works fine if you extract the child into a variable:

child = <B {...params} />
<A {...{child}} />

I think the issue is still to do with that csxObjAttribute field - in the presence of arbitrary nested mixes of {...*} and =* we would need a stack to keep track of what the lexer is currently scanning in order to correctly match } or ).

connec · 2017-08-02T16:38:14Z

Another failing example:

<A {...{child: <B foo={bar} />}} />

The problem occurs because:

The lexer sees { without : and sets csxObjAttribute = yes.
The lexer sees { with : and sets csxObjAttribute = no.
The lexer sees } with not csxObjAttribute and emits a ) to close the field.
The lexer sees } with not csxObjAttribute and emits a ) to close the field - this is the incorrect step that needs to instead emit }.

The story with <A {...{child: <B {...params} />}} /> is similar:

The lexer sees { without : and sets csxObjAttribute = yes.
The lexer sees { without : and sets csxObjAttribute = yes.
The lexer sees } with csxObjAttribute and emits a } to close the field, and sets csxObjAttribute = no.
The lexer sees } with not csxObjAttribute and emits a ) to close the field - this is the incorrect step that needs to instead emit }.

zdenko · 2017-08-02T19:46:00Z

@connec good catch. I improved tracking of the splats inside the CSX tag.

GeoffreyBooth · 2017-08-03T02:22:57Z

@connec Looks like the latest commit fixed both examples:

// <A {...{child: <B {...params} />}} />
<A {...{
    child: <B {...params} />
  }} />;


// <A {...{child: <B foo={bar} />}} />
<A {...{
    child: <B foo={bar} />
  }} />;

And ditto with the dots on the right side.

connec

Nice 👍

# Conflicts: # lib/coffeescript/lexer.js # lib/coffeescript/nodes.js # src/nodes.coffee

GeoffreyBooth · 2017-08-03T04:00:55Z

Great work @zdenko!

Zdenko Vujasinovic and others added 3 commits July 8, 2017 14:26

CSX spread attributes: <div {props...} />

da086b9

whitespace cleanup

8841e47

Style

a6172c0

GeoffreyBooth changed the title ~~CSX spread attributes: <div {props...} />~~ [CS2] CSX spread attributes: <div {props…} /> Jul 9, 2017

GeoffreyBooth added the enhancement label Jul 9, 2017

GeoffreyBooth added this to the 2.0.0 milestone Jul 9, 2017

valid CSX attributes

c62be6b

GeoffreyBooth mentioned this pull request Jul 19, 2017

[CS2] Clarify the project status #4620

Closed

connec reviewed Jul 23, 2017

View reviewed changes

added comments; cleanup

91a294b

Fixed allowed CSX properties.

4a318fc

Cleanup

2d06fb1

GeoffreyBooth requested changes Jul 24, 2017

View reviewed changes

cleanup

437ff70

zdenko force-pushed the csx-spreads branch from 7af23b4 to 437ff70 Compare July 28, 2017 17:51

tests

b097188

connec reviewed Jul 31, 2017

View reviewed changes

fix nested assignement; remove unused variable

e86285b

GeoffreyBooth requested changes Aug 2, 2017

View reviewed changes

cleanup; improve tests

7de2eb9

GeoffreyBooth approved these changes Aug 2, 2017

View reviewed changes

fix esoteric case; improve tracking nested splats in CSX tag

ff8d4f2

connec approved these changes Aug 3, 2017

View reviewed changes

Merge branch '2' of github.com:jashkenas/coffeescript into csx-spreads

bd3fd67

# Conflicts: # lib/coffeescript/lexer.js # lib/coffeescript/nodes.js # src/nodes.coffee

GeoffreyBooth merged commit cbf035f into jashkenas:2 Aug 3, 2017

GeoffreyBooth deleted the csx-spreads branch August 3, 2017 04:00

GeoffreyBooth changed the title ~~[WIP][CS2] CSX spread attributes: <div {props…} />~~ [CS2] CSX spread attributes: <div {props…} /> Aug 3, 2017

This was referenced Aug 3, 2017

[CS2] Support for CSX - equivalent of JSX #4551

Merged

CS2 Discussion: Project: Progress as of August 2017: 2.0.0-beta4! coffeescript6/discuss#87

Closed

coffeescriptbot mentioned this pull request Feb 19, 2018

CS2 Discussion: Project: Progress as of August 2017: 2.0.0-beta4! #4980

Closed

[CS2] CSX spread attributes: <div {props…} /> #4607

[CS2] CSX spread attributes: <div {props…} /> #4607

Conversation

zdenko commented Jul 8, 2017

GeoffreyBooth commented Jul 9, 2017

zdenko commented Jul 9, 2017 • edited Loading

xixixao commented Jul 9, 2017

GeoffreyBooth commented Jul 9, 2017

zdenko commented Jul 9, 2017

GeoffreyBooth commented Jul 9, 2017

zdenko commented Jul 9, 2017

GeoffreyBooth commented Jul 9, 2017

connec commented Jul 10, 2017 • edited Loading

zdenko commented Jul 12, 2017

zdenko commented Jul 19, 2017

GeoffreyBooth commented Jul 19, 2017

GeoffreyBooth commented Jul 23, 2017

Choose a reason for hiding this comment

connec commented Jul 24, 2017

microdou commented Jul 24, 2017 • edited Loading

zdenko commented Jul 24, 2017

zdenko commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microdou commented Jul 28, 2017

microdou commented Jul 28, 2017

GeoffreyBooth commented Jul 28, 2017

connec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microdou commented Jul 31, 2017

GeoffreyBooth commented Aug 1, 2017

microdou commented Aug 1, 2017

GeoffreyBooth commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth commented Aug 2, 2017

connec commented Aug 2, 2017

connec commented Aug 2, 2017

zdenko commented Aug 2, 2017

GeoffreyBooth commented Aug 3, 2017

connec left a comment

Choose a reason for hiding this comment

GeoffreyBooth commented Aug 3, 2017

zdenko commented Jul 9, 2017 •

edited

Loading

connec commented Jul 10, 2017 •

edited

Loading

microdou commented Jul 24, 2017 •

edited

Loading