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 Discussion: Output: Destructuring #4962

Closed
coffeescriptbot opened this issue Feb 19, 2018 · 16 comments
Closed

CS2 Discussion: Output: Destructuring #4962

coffeescriptbot opened this issue Feb 19, 2018 · 16 comments

Comments

@coffeescriptbot
Copy link
Collaborator

coffeescriptbot commented Feb 19, 2018

From @GeoffreyBooth on 2017-01-23 03:58

Building off of coffeescript6/discuss#18, we should consider changing CoffeeScript’s destructuring output from the current ES3 to ESNext syntax. So this:

numbers = [1, 2, 3]
pairs = { a: 1, b: 2, c: 3 }

[one, two, three] = numbers
{one, two, three} = pairs

foo = ->
  [@a, @b, @c] = numbers
  {@a, @b, @c} = pairs
  return

which currently compiles to this:

var foo, numbers, one, pairs, three, two;

numbers = [1, 2, 3];

pairs = {
  a: 1,
  b: 2,
  c: 3
};

one = numbers[0], two = numbers[1], three = numbers[2];

one = pairs.one, two = pairs.two, three = pairs.three;

foo = function() {
  this.a = numbers[0], this.b = numbers[1], this.c = numbers[2];
  this.a = pairs.a, this.b = pairs.b, this.c = pairs.c;
};

would instead compile to this (?):

var foo, numbers, one, pairs, three, two;

numbers = [1, 2, 3];

pairs = {
  a: 1,
  b: 2,
  c: 3
};

[one, two, three] = numbers;
({one, two, three} = pairs);

foo = function() {
  [this.a, this.b, this.c] = numbers;
  ({a: this.a, b: this.b, c: this.c} = pairs);
};

The array destructuring, as you can see here, is pretty straightforward; but the object destructuring presents issues. First, if we’re not declaring the variable at the same time (as we never do in CoffeeScript, hence the var line of declarations at the top of each scope) then we need to wrap the destructuring assignment in parentheses, per MDN. And if the target variable is attached to this, we need to use the “assigning to new variable names” shortcut to destructure into our intended destination variable, unless people have a better suggestion for how to handle this.

There’s also the question of whether this is worth the effort. The current output is already rather readable, so the gains aren’t great. Destructuring, whether in CS or ES, is just a time-saver; anything you can do with destructuring you can also already do in more verbose variable assignment, so there’s no compatibility need for us to change anything. If anything, the lack of need behooves us to make sure that if we make this change, it is done in a fully backward compatible way.

Edit: Fixed destructuring object with this per @lydell’s correction.

@coffeescriptbot
Copy link
Collaborator Author

From @lydell on 2017-01-23 06:46

Don't you mean this for destructuring objects with @:

({a: this.a, b: this.b, c: this.c} = pairs);

Note that there is one big difference between CS destructuring and ES. Generators. [a, b] = generator() pulls the first two values out of a generator in ES, but not in CS. So I think changing the output is needed.

I can't see any problems with your suggested compilation (other than what I mentioned above).

@coffeescriptbot
Copy link
Collaborator Author

From @GeoffreyBooth on 2017-01-23 06:54

@lydell Thanks, you’re correct. Updated. I guess this means that no temporary variables are required? That simplifies things.

Not that I don’t want to do this, but it’s not like supported ES destructuring is required. Your example could simply be written as:

a = generator()
b = generator()

Though one could argue that anything Babel transpiles can be written in ES3 in some form or another. But this edge case is easier (and less boilerplatey) than most.

@coffeescriptbot
Copy link
Collaborator Author

From @lydell on 2017-01-23 07:03

You mean this:

gen = generator()
a = gen.next().value
b = gen.next().value

Anyhow, your suggested output change seems totally correct, and seems to only bring benefits (full ES compatibility for free, nicer code outout), so I can't see why we would not do this.

@coffeescriptbot
Copy link
Collaborator Author

From @GeoffreyBooth on 2017-01-23 07:22

It’s not that we wouldn’t do this, it’s more of a question of priority. Like I think we can release CS2-alpha1 without this being ready (but not before #4424 I think).

Are there any edge cases I’m not thinking of? The MDN page has some interesting things like default values, skipped values, matches via regex, etc.

@coffeescriptbot
Copy link
Collaborator Author

From @lydell on 2017-01-23 07:51

  • Default values work just like parameter defaults. Should be no problem.
  • Skipped values: [first, , third] = [1, 2, 3]. Good point. CoffeeScript currently doesn’t allow this. Not sure what to do. Shouldn’t hold up a PR implementing this change, though: It’s kind of a separate thing.
  • Matches via regex: There is no such thing. MDN only gives an example on how someRegex.exec returns an array and it can be useful to destructure that array ()
  • Priority: I agree, definitely no rush on this one.

@coffeescriptbot
Copy link
Collaborator Author

From @connec on 2017-01-23 11:15

For @-arguments, we'll need to maintain the current codepath (in 2) that extracts this assignments for derived constructors (unless we drop that). I.e. this:

class B extends A
  constructor: (@name) ->
    super

Should continue to compile to:

class B extends A {
  constructor (name) {
    super(...arguments)
    this.name = name
  }
}

Otherwise this could potentially clean up a lot of code!

@coffeescriptbot
Copy link
Collaborator Author

From @lydell on 2017-01-23 11:30

@connec Do you mean this?

class B extends A
  constructor: ({@name}) ->
    super

class B extends A {
  constructor(arg) {
    super(...arguments)
    ({name: this.name} = arg);
  }
}

@coffeescriptbot
Copy link
Collaborator Author

From @connec on 2017-01-23 12:56

Whoops, @lydell, I do indeed 😅

@coffeescriptbot
Copy link
Collaborator Author

From @edemaine on 2017-01-23 13:44

Skipped values in array destructuring seems like a cool feature. I think ideally that could be added to CS1 and CS2. Perhaps worth forking off into a separate issue, at least for CS1.

@coffeescriptbot
Copy link
Collaborator Author

From @JavascriptIsMagic on 2017-01-24 20:04

If I need skipped values I usually just assign to a veriable I won't use:

[a, _, _, b] = [1, 2, 3, 4]

Also there is another edge case from coffeescript that I do not believe ES6 covers?

[a, ..., b] = [1, 2, 3, 4]

I don't think the ... even makes sense in the context of iterables like generators:

[a, ..., b, c, d, e] = generator()

If the generator only emits 4 values or infinite values, what does the above snippet mean?

CS1 is assuming we are working with an [array-like], but if I am not mistaken, Javascript has been moving towards favoring iterable support in places CS1 previously could assumed where [array-like] things such as for.

@coffeescriptbot
Copy link
Collaborator Author

From @edemaine on 2017-01-24 21:39

@JavascriptIsMagic _ is a fine idiom until you're using Underscore. So I think skipping values would still be useful, though minor.

Agreed, those ... examples seem impossible/meaningless with iterators. I think those examples should compile as-is (meaning they assume array-like), while destructuring without ... could be compiled to ES6 which would work with arrays or iterators.

@coffeescriptbot
Copy link
Collaborator Author

From @GeoffreyBooth on 2017-02-05 01:13

@jashkenas or @lydell am I correct in understanding that the current destructuring implementation is in Assign::compilePatternMatch? This is a 107-line-long function . . . care to explain what’s going on here? Any general advice regarding how to refactor/replace this to output ES destructuring?

I have a hunch that this will turn into something similar to updating function parameters, where we can output ES syntax in some cases but need to fall back to the 1.x implementation for cases like expansions that aren’t compatible with ES. So maybe this function can shrink a little with outputting destructuring as ES, but big chunks of complexity will likely remain.

@coffeescriptbot
Copy link
Collaborator Author

From @lydell on 2017-02-05 10:40

Yes, that's the place. I added some quick comments to it for you: lydell@590cd3f

  • [a, ..., b] = c: I guess we could compile them to splats? var [a, ...hiddenVariable, b] = c.
  • {@a} = b: I guess we can use var {a: this.a} = b?
  • [splat..., b] = c: Not sure.

Also, it might be worth thinking about if we want to start supporting {a, b...} = c.

@coffeescriptbot
Copy link
Collaborator Author

From @connec on 2017-02-06 09:59

Does this mean you're working on destructuring @GeoffreyBooth?

@coffeescriptbot
Copy link
Collaborator Author

From @GeoffreyBooth on 2017-02-06 16:09

I’ve started a branch, but not really done any work yet: https://github.com/GeoffreyBooth/coffeescript/tree/destructuring

Feel free to work on this branch if you’d like. I’m not going to hold up CS2-alpha1 for this feature, though.

@coffeescriptbot
Copy link
Collaborator Author

From @GeoffreyBooth on 2017-03-29 04:58

I’ve opened a pull request for discussion of that branch: #4478

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

No branches or pull requests

1 participant