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

cell destructuring #88

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

cell destructuring #88

wants to merge 10 commits into from

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Dec 4, 2019

Destructuring allows a cell to expose multiple values.

({foo, bar}) = ({foo: 1, bar: 2})
({foo, bar}) = { return {foo: 1, bar: 2}; }
([foo, ...bar]) = [1, 2, 3]

This PR allows the cell.id to be an ObjectPattern or an ArrayPattern. There’s still a fair amount of work on the compiler side to turn this into the corresponding set of variables, but this serves as the language proposal.

TODO

  • tests
  • support default values in array patterns
  • support default values in object patterns
  • capture references to other cells in default value expressions
  • capture features from default value expressions
  • don’t allow await or yield inside default value expressions
  • don’t allow destructuring into comma expressions

@mbostock mbostock requested a review from jashkenas December 4, 2019 20:20
@jashkenas
Copy link
Contributor

This initial implementation looks good, and the functionality it provides is highly desirable.

While there are going to be a few small considerations to keep in mind in the UI when the adopt destructuring (permalink names for cells, display of multiple names in the cell menu, embed code generation for a cell that exposes multiple values...), those are all fairly easily surmountable.

I'd still like to register my (already verbally discussed) mild objection to this syntax. We’re appropriating the parenthetical declaration of destructured function arguments, instead of the JavaScript canonical parenthetical wrapping of the entire destructuring assignment. I'd prefer that we hew to JavaScript if we’re going to add this.

For example, JavaScript:

let a, b;
...
({a, b} = expression);

But in this proposal:

({a, b}) = expression

I'd prefer:

({a, b} = expression)

... and of course, dropping the parentheses altogether would be best.

There’s quite a bit more discussion about the ES6 parse choices of ({a, b}) = ... vs ({a, b} = ...) in this GitHub thread, as well as the special note in the ES spec forbidding expression statements from starting with {.

@mbostock
Copy link
Member Author

mbostock commented Dec 4, 2019

Thanks for the references and feedback!

It feels unfair (to me) to say that one of these approaches “hews to JavaScript” while the other doesn’t. Named cells are not assignment expressions; they declare a binding between one or more identifiers and a function body, the latter being invoked automatically by the runtime. In my view cells are closest to arrow functions.

({foo, bar}) => body

The difference is that the name of the cell binds to the output rather than the input. Hence, my proposal to put the parens around the binding.

({foo, bar}) = body

The other (lexical binding) precedent to consider is a variable declaration.

let {foo, bar} = expression;

We could use a keyword to make it immediately clear whether a cell is named and avoid ambiguity. That could be a new keyword (such as cell) or the exist const keyword.

const {foo, bar} = body

Simple names could be written this way, too, if desired.

const foo = body

And you could possibly have a const mutable, which is a little weird.

const mutable foo = body

The reason I’ve avoided a keyword (from the beginning) is that it feels like a lot of noise for very little signal. Cells are already delineated as separate scripts, and in the common case only a couple tokens are needed to determine whether a cell is named. Introducing a new keyword to support destructuring feels more disruptive than necessary.

@mbostock
Copy link
Member Author

mbostock commented Dec 4, 2019

I realized that both of these proposals

(foo) = body
(foo = body)

are potentially not backwards-compatible with mutable assignments, though my guess there are few (if any) cells that would break. In the parens-around-binding proposal, a mutable assignment could change to a mutable declaration.

(mutable foo) = body

In the parens-around-cell proposal, a mutable assignment could change to a mutable declaration, too.

(mutable foo = body)

Also, in the parens-around-binding proposal, previously invalid syntax (assignment to constant variable foo) could become a valid cell declaration.

(foo) = 42

This is true of the parens-around-cell proposal, too.

(foo = 42)

Personally, I find the parens-around-cell proposal looks more like an assignment expression and less like a lexical binding, and hence I prefer the parens around the binding. 😄

@mbostock mbostock marked this pull request as ready for review December 25, 2021 00:30
@mbostock mbostock marked this pull request as draft December 28, 2021 03:56
@mbostock mbostock marked this pull request as ready for review December 28, 2021 18:43
@mbostock mbostock changed the title Allow destructuring of cells. cell destructuring Dec 29, 2021
Copy link
Contributor

@sydneypalumbo sydneypalumbo left a comment

Choose a reason for hiding this comment

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

Amazing! All the test output yielded what I would expect. 👍 Glanced at the related compiler updates you're working on, but curious to dig more into that, too.

@CobusT
Copy link

CobusT commented Feb 13, 2022

Any chance that this can be merged? Just going through the feedback repo and I think this would close observablehq/feedback#48

@selenecodes
Copy link

Honestly can't wait for this feature to be released.

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

Successfully merging this pull request may close these issues.

6 participants