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.

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

[CS2] Compile computed properties to ES2015 equivalent #4338

Merged
merged 3 commits into from
Oct 24, 2016
Merged

[CS2] Compile computed properties to ES2015 equivalent #4338

merged 3 commits into from
Oct 24, 2016

Conversation

connec
Copy link
Collaborator

@connec connec commented Oct 15, 2016

This is a fairly small change that simplifies the code generation for computed properties as they're now generated in the object initializer like regular properties.

I ended up doing this whilst working on fully switching to ES2015 splats. Targets the 2 branch since the output won't compile without ES2015 support.

Chris Connelly added 2 commits October 15, 2016 23:06
This is a fairly small change that simplifies the code generation for computed
properties as they're now generated in the object initializer like regular
properties.
@connec
Copy link
Collaborator Author

connec commented Oct 15, 2016

Also, the first commit is actually just compiling some files in 2 that weren't committed for some reason.

if prop not instanceof Assign
prop = new Assign prop, prop, 'object'
if prop not instanceof Comment and prop not instanceof Assign
if prop.isComplex()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously, dynamic keys were being detected with...

(prop.variable or prop).base instanceof Parens

... but this seems simpler. I don't know all the ins and outs though, so I might be missing something.

@jashkenas
Copy link
Owner

Nice stuff!

@GeoffreyBooth
Copy link
Collaborator

@connec this looks good, do you mind just giving an example of some CoffeeScript with computed properties, and what it was compiling to before and after this PR?

@lydell
Copy link
Collaborator

lydell commented Oct 21, 2016

@GeoffreyBooth Here's an example: 76c076d

@connec
Copy link
Collaborator Author

connec commented Oct 21, 2016

Sure:

Current (1.11.1):

★ coffee -bcs
{
  "#{computed}": 1
  "#{f()}"
  normal: 2
  "#{a.bit.more.complex = computation()}": 3
}
// Generated by CoffeeScript 1.11.1
var obj, ref;

(
  obj = {},
  obj["" + computed] = 1,
  obj[ref = "" + (f())] = ref,
  obj.normal = 2,
  obj["" + (a.bit.more.complex = computation())] = 3,
  obj
);

In this PR:

★ bin/coffee -bcs
{
  "#{computed}": 1
  "#{f()}"
  normal: 2
  "#{a.bit.more.complex = computation()}": 3
}
// Generated by CoffeeScript 1.11.0
var ref;

({
  ["" + computed]: 1,
  [ref = "" + (f())]: ref,
  normal: 2,
  ["" + (a.bit.more.complex = computation())]: 3
});

@GeoffreyBooth
Copy link
Collaborator

This looks good to me. I updated your branch with the latest 2, and the tests pass. Anyone opposed to merging this in?

@GeoffreyBooth GeoffreyBooth modified the milestone: 2.0.0 Oct 23, 2016
@GeoffreyBooth GeoffreyBooth changed the title Compile computed properties to ES2015 equivalent [CS2] Compile computed properties to ES2015 equivalent Oct 23, 2016
@lydell
Copy link
Collaborator

lydell commented Oct 23, 2016

Go for it!

@danielbayley
Copy link
Contributor

Following on from #3597, wouldn't it now be preferable to simply allow the existing ES6 [key]: value syntax and pass it straight through for this one?

@connec
Copy link
Collaborator Author

connec commented Nov 26, 2016

@danielbayley the purpose wasn't to add or change any syntax, interpolations were already allowed in object keys, this just simplifies the output by compiling to ES6. Might be worth having a separate discussion about [key] syntax.

Personally I prefer CS' approach to this. If "hello" is a valid key, then so too should be "#{hello}". ES' handling of template strings is needlessly restrictive imo.

@danielbayley
Copy link
Contributor

danielbayley commented Nov 26, 2016

interpolations were already allowed in object keys

@connec Is this what you were referring to?

key = 'key'

obj = {}
obj[key] = 'value'

Obviously this is nothing new, but just to clarify…

"#{key}": 'value' # works… cool! Leave it be.

[key]: 'value' # is valid ES6 but I think should also work in CS.

Maybe you're right about it being a separate discussion; is it worth opening another issue?

@connec
Copy link
Collaborator Author

connec commented Nov 26, 2016

interpolations were already allowed in object keys

@connec Is this what you were referring to?

Sorry, I just meant that this syntax was valid before the PR:

key = 'key'
obj =
  "#{key}": 'value'

It used to compile to (roughly):

var key, obj

key = 'key';

obj = (
  ref = {},
  ref['' + key] = 'value',
  ref
);

And now compiles to:

var key, obj

key = 'key';

obj = {
  ['' + key]: 'value'
};

is it worth opening another issue?

I'm not sure, @GeoffreyBooth may have an opinion on this.

@GeoffreyBooth
Copy link
Collaborator

Personally I don’t see why [key]: 'value' shouldn’t be valid CoffeeScript. @lydell? I think this corresponds with our discussing allowing ... on either side of a variable name, e.g. ...args or args..., to ease the transition for people who write lots of ES.

Perhaps open a new issue requesting the new syntax. Please prefix it with [CS2], as I don’t think we’re revisiting this anymore in 1.x.

@lydell
Copy link
Collaborator

lydell commented Nov 27, 2016

I guess we more or less have to support [key]: 'value' to allow stuff like [Symbol.iterator]: foo.

@connec
Copy link
Collaborator Author

connec commented Nov 27, 2016

[Symbol.iterator]: foo

Good point 😢

I still think it's consistent to allow interpolations in keys though.

@lydell
Copy link
Collaborator

lydell commented Nov 27, 2016

I still think it's consistent to allow interpolations in keys though.

Me too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants