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

Sub-structuring #4995

Closed
wants to merge 3 commits into from
Closed

Sub-structuring #4995

wants to merge 3 commits into from

Conversation

zdenko
Copy link
Collaborator

@zdenko zdenko commented Feb 28, 2018

This PR adds features proposed in #4991: assigning properties from one object into another object (i.e. building / extending objects).

data =
  title: "Mr"
  name: "Coffee"
  address: "Sesame Street"

view = data{title, name:full_name}
# view = { title: "Mr", full_name: "Coffee" }

view = data{title, name:full_name, rest...}
###
view = {
  title: "Mr", 
  full_name: "Coffee",
  rest: {
    address: "Sesame Street"
  }
}
###

console.log  data{title, name:full_name}
#  { title: "Mr", full_name: "Coffee" }

data =
  title: "Mr"
  name: "Coffee"
  address: "Sesame Street"

view{title, name:full_name} = data
###
error: the variable 'view' has not been declared before
view{title, name:full_name} = data
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
###

data =
  title: "Mr"
  name: "Coffee"
  address: "Sesame Street"

view = address: "Elm Street"
view{title, name:full_name} = data
###
view = { address: 'Elm Street', title: 'Mr', full_name: 'Coffee' }
###

data =
  title: "Mr"
  name: "Coffee"
  address: "Sesame Street"
  zip: 10001

view = city: "London"
view{title, name, -zip, rest...} = data
# view = {city: "London", title: "Mr", name: "Coffee", rest: {address: "Sesame Street"}}

data =
    addr:
      street: ["Elm", 12]
      city: "London"
      state: "England"
      zip: "GH 201 X"
      floor: 30
      appartment: "XJ-1"
    phone:
      home: "999-555-111"
      work: "888-333-555"

  user = name: "CS"
  user{
    addr: {
      street: [street1, street2],
      city, state, -zip, -floor, ext...
    },
    phone: { home:phone_home }
  } = data

###
user = {
  name: "CS",
  street1: "Elm",
  street2: 12,
  city: "London",
  state: "England",
  ext: {
    appartment: "XJ-1"
  },
  phone_home: "999-555-111"
}
###


user = data{
      addr: {
        street: [street1, street2],
        -zip, -floor, ext...
      },
      phone: { home:phone_home }
    }

###
user = {
  street1: "Elm",
  street2: 12,
  ext: {
     city: "London",
     state: "England",
     appartment: "XJ-1"
  },
  phone_home: "999-555-111"
}
###

@vendethiel
Copy link
Collaborator

-1 for "Excluded properties". I don't think we should introduce new syntax, instead we should use e.g. {x: []} or {x: {}} or {x: null} or {x:,} or...

Copy link
Collaborator

@vendethiel vendethiel left a comment

Choose a reason for hiding this comment

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

Nice work, as usual

src/nodes.coffee Outdated
getProps = (props) => (p for p in @finalProps @objProperties props)
for prop in props
if prop instanceof Obj
ret = [ret..., (getProps prop.properties)...]
Copy link
Collaborator

Choose a reason for hiding this comment

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

we usually write calls getProps(prop.properties)

src/nodes.coffee Outdated

finalProps: (props = []) ->
ret = []
getProps = (props) => (p for p in @finalProps @objProperties props)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the for/in here? it's not here L792

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I forgot to remove it. It's a leftover from the previous version.

@zdenko
Copy link
Collaborator Author

zdenko commented Mar 1, 2018

I agree with @vendethiel regarding the "Excluded properties".
x: {} is much more readable and already does the same.

@zdenko zdenko changed the title WIP: Sub-structuring Sub-structuring Mar 4, 2018
@aminland
Copy link

aminland commented Mar 7, 2018

Nice work @zdenko !

Soo I'm just looking at what babel ends up doing when you destructure with {a:{}} = {a:2}:

function _objectDestructuringEmpty(obj) { if (obj == null) throw new TypeError("Cannot destructure undefined"); }
var _a = { a: 2 };
_objectDestructuringEmpty(_a.a);
_a;

So basically it will throw an error if the key doesn't exist, which imho is a poor design choice by the tc39 folks. The only use that it provides is basically a shorthand for validation.

The reason I like the -key syntax is that it is concise way to say you don't care about certain properties (not that you want errors thrown). It is also, in the vain of everything else coffeescript, semantically clear. Together with the rest of the PR, it would allow full elimination of _.pick and _.omit, which are two of the most used lodash functions...

That's not to say we should prevent folks from doing the {a:{}} syntax, but at least this way people can have an option to be deliberate about throwing runtime errors. Note that regular destructuring does not cause babel to check that something is defined (i.e. {a,b}=c is a straightforward conversion, with no possible exceptions stemming from key choice). Thus, the js behavior is not really consistent, and it would behoove us to right some JS wrongs ;)

@zdenko
Copy link
Collaborator Author

zdenko commented Mar 8, 2018

@aminland -key syntax is not removed from this PR. It compiles into key:{}, which is a valid ES syntax for dropping keys. Besides, once #4884 is merged, it will work without a change. Otherwise, we would have to keep parts of the current polyfill from the Access in order to remove keys.
Also, the -key syntax is mostly useful with rest destructuring  {a, -b, r...}. I see no point compiling {a, -b} into {a}. And how to deal with this case {-a}?
Now, let's presume that o = {b:1} and user writes {a:{}, b} = o. The type error will enable him to correct the mistake, whose cause can be somewhere else, e.g. result from the function from some module. I think we should allow the user to shoot himself in the foot.
If my intention is to remove some keys, I have to be sure these keys exist in the first place or use a third-party library or custom function to safely remove unwanted keys from the object.

@GeoffreyBooth
Copy link
Collaborator

Sorry to join this discussion so late, I’m just starting to wrap my head around this. To take your initial example:

data =
  title: "Mr"
  name: "Coffee"
  address: "Sesame Street"

view = data{title, name:full_name}
# view = { title: "Mr", full_name: "Coffee" }

So the view = assignment line is basically a shorthand for:

view = Object.assign {}, {title: data.title, full_name: data.name}

Which gives me two questions:

  1. Obviously the “sub-structuring” syntax is shorter, but is this enough of an improvement to be worth adding? Why?
  2. Why is it view = data{name: full_name} and not view = data{full_name: name}? The latter feels more natural to me, as when you’re assigning to a variable in general the keys for the object in the new variable go on the left, e.g. view = {full_name: "Coffee"}. See also the Object.assign example.

@zdenko
Copy link
Collaborator Author

zdenko commented Apr 6, 2018

Why is it view = data{name: full_name} and not view = data{full_name: name}

This syntax is the same as assigning to new variable names in object destructuring, i.e. the new name is on the right side.

o = {p: 42, q: yes}
{p: foo, q: bar} = o
# foo = 42, bar = yes

@aminland
Copy link

aminland commented Apr 6, 2018

Yeah I think JS kind of messed up by choosing that syntax... but since it's already been implemented in CS and it's here to stay, we have to go with it. The idea here is that it's not a new syntax really, as much as it is an extension of the existing destructuring syntax to make it more useful.

@GeoffreyBooth to answer your question 1, I certainly think so. Often (esp. for react components) I'm finding myself having to extract and pass through dozens of props all the time, and this gets very tedious.
Sure if you're just trying to get 1 variable out, you're not saving much.
If you're getting a lot of things out, or alternately are trying to update an existing variable by using the lhs version, you not only save a lot of typing, but also make the code more concise and clear as to what you're trying to do.

@GeoffreyBooth
Copy link
Collaborator

Closing for now per #4991 (comment)

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

Successfully merging this pull request may close these issues.

4 participants