Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Destructuring syntax #74

Closed
webdif opened this issue Nov 14, 2018 · 35 comments
Closed

Destructuring syntax #74

webdif opened this issue Nov 14, 2018 · 35 comments

Comments

@webdif
Copy link

webdif commented Nov 14, 2018

How about:

const obj = {
  foo: {
    bar: {
      baz: 42,
    },
  },
};

// const baz = obj?.foo?.bar?.baz; 
const { baz } = obj?.foo?.bar?;

It would be a ? operator, so it's certainly out of scope.
But it would be very nice and intuitive to have such a syntax in my humble opinion, so here's the issue.

@RusinovAnton
Copy link

Lets go wild

const { foo: ?{ bar: ?{ baz } } } = obj?.;

@webdif
Copy link
Author

webdif commented Nov 14, 2018

Oh, I just stumbled upon this: #6
But answers are not satisfactory, with even worse syntax…

@claudepache
Copy link
Collaborator

But it would be very nice and intuitive to have such a syntax in my humble opinion, so here's the issue.

Is it intuitive?

let obj = null;
let baz = 2;
({ baz } = obj?.foo?.bar?);
console.log(baz);

Should it log 2 or undefined?

IOW, is it optional assignment of a value:

let _ =  obj?.foo?.bar;
if (_ != null)
   ({ baz } = _);

or assignment of an optional value:

({ baz } = obj?.foo?.bar ?? { });
// or
({ foo: { bar: { baz } = { } } = { } } = obj || { });

?

@webdif
Copy link
Author

webdif commented Nov 14, 2018

I think it would be assignment of an optional value, and it should log undefined.

I did not know about ??, but I think this is pretty nice:

const { baz } = obj?.foo?.bar ?? { };

It's not as handy as:

const { baz } = obj?.foo?.bar?;

But it's clean, short enough, and baz is not repeated twice 👍

@Whobeu
Copy link

Whobeu commented Nov 14, 2018

Coming from 12 years of C# I am used to its newer "??" and Safe Navigation "Object?" syntax. While I agree that some of the destructuring examples can create some non-intuitive code, the power unleashed to the programmer is quite interesting.

Of course such arguments over language changes are not unique to JS. Python had a recent furor over proposed syntax changes to allow Assignment Expressions (seems kind of silly with all the furor over that one) resulting in the creator stepping down. Perl has had ongoing arguments for years over Smartmatch "~~" and still calls it experimental.

@zfrisch
Copy link

zfrisch commented Nov 15, 2018

({ baz } = obj?.foo?.bar ?? { });

Just to clarify, this is basically giving us a default value outside from undefined? So if for any reason this object (obj.foo.bar) isn't defined entirely and optional chaining falls through, it'll return an empty object?

@dusave
Copy link
Member

dusave commented Nov 15, 2018

@zfrisch theoretically, yes. However, ?? is part of a different proposal which is only loosely tied to this one. As it stands today, that is the behavior.

@armanozak
Copy link

‪I like that syntax a lot and actually think it will lead to an optional destructuring syntax like the following in near future:

‪const {foo ?: {bar ?: {baz}}} = obj?

IMHO, this is safe, readable, and inline with the optional chaining syntax.

@littledan
Copy link
Member

You can do something similar using destructuring defaults: https://mobile.twitter.com/mfpiccolo/status/1080193719637143552

I am not convinced it would be helpful to add this feature. It seems like a lot of extra stuff just for null handling and the intersection of these two features. I think it would make code more confusing.

@armanozak
Copy link

Is optional chaining not all about null/undefined handling? After all, we could already do sth. like that:

const prop = ((foo || {}).bar || {}).baz;

P.S. Altough I do understand what you mean by destructuring defaults, I could not understand why you referred to that tweet. It has nothing about destructuring defaults. It is just another syntax proposal for optional destructuring. If you mean you find the syntax proposed in the tweet is confusing, I agree. But I also believe we can have a better syntax.

@littledan
Copy link
Member

I mean, defaults handle undefined and not null. Anyway, I wonder if ?? will handle many of the use cases in this feature request, for when you do want null support.

@TylerBarnes
Copy link

const {
  foo?: {
    bar?: { baz }
  }
} = obj;

would be so nice!

@mitchellsimoens
Copy link

Optional chaining is syntactical sugar, yes. You can get the same result as the snippet @armanozak posted:

const prop = ((foo || {}).bar || {}).baz;

But the optional chaining syntax is much more readable. That snippet is "ugly" and the more nested the uglier it gets because you have to start wrapping things in parentheses more and more.

So while default values in destructuring can get you the end result:

const { foo: { bar: { baz } = { } } = { } } = obj || { };

It's not as readable as optional destructure chaining could be:

const {
  foo?: {
    bar?: { baz }
  }
} = obj;

I know... some people are likely thinking "where will it end?!" but you have others saying "why pick and choose what you throw sugar on?" Destructuring is sugar over chaining so if chaining has sugar to handle undefined properties, why shouldn't destructuring?

@TylerBarnes
Copy link

TylerBarnes commented Mar 12, 2019

Definitely. To add to that, sugar that makes code easier to read, write, and reason about is inherently valuable due to time savings and reduced mental load.

@rijnhard
Copy link

So I thought this could be used to replace deep object merging, but it can't.
So in use case I find it a bit limited,

  • for extraction a single value from an object const { baz } = obj?.foo?.bar?; already works fine.
  • for defaulting multiple fields inside deep objects (Example B), something like lodash.merge works better, and I'd be hard pressed to say Example A works at all, it only has a sweet spot, on of extracting a few variables.
// Example A
const {
  foo?: {
    bar?: { baz }
  }
} = obj;
// Example B
function myfunc(options = {}) {
    options = _.merge({
        foo: {
            bar: {
                baz: true
            }
        },
        x: {
            y: {
                z: true,
                y: true
            },
            z: false
        },
    });
    // carry on
}

Although this argument is compelling

I know... some people are likely thinking "where will it end?!" but you have others saying "why pick and choose what you throw sugar on?" Destructuring is sugar over chaining so if chaining has sugar to handle undefined properties, why shouldn't destructuring?

The counter argument is everything we say yes to, means something else we say no to, we just may not know what is yet.

Adding simple things with limited value now, then we block ourselves from future functionality and it's hard to imagine because we don't know what that is.

My vote:

// YES, for clear value
const { baz } = obj?.foo?.bar?;

// NO, for no clear value
const {
  foo?: {
    bar?: { baz }
  }
} = obj;

@TylerBarnes
Copy link

@rijnhard I would say the use case isn't for merging objects, it's for grabbing more than one deeply nested property from the same object.

For your second example this is why it would be useful:

const {
  foo?: {
    bar?: { baz }
  },
  whatIfI?: {
    also?: {
      want?: {
        thisProperty
      }
    }
  },
  and?: { 
   thisOne,
   asWellAs?: { thisOtherOne }
  }
} = obj;

This is an issue I run into in React frequently. An API returns a complex object and I need to write all kinds of checks around accessing deeply nested properties. It's messy looking and not nice to read.
TBH, The above example is still not that nice to read, but it's a lot nicer – and it's simpler to write.

There are cases where I would have to write 20+ lines of this:

const { baz } = obj?.foo?.bar?;
const { thisProperty } obj?.whatIfI?.also?.want;
const { thisOne } = obj?.and;
const { thisOtherOne } obj?.and?.asWellAs;

@kaizhu256
Copy link

-1, because its not javascripty.

javascript should stop promoting language-features that encourage bad-habits like nesting JSON-properties (which complicate UX-workflow transformation/validation/data-passing). the idiom of using flat-and-descriptive property-names over short-and-nested/overloaded ones should prevail.

also, 20+ lines of the above-example is perfectly readable (plus making it a chore/pita to write it out serves as early-warning that something is wrong with your [nested] UX-workflow design).

const { baz } = obj?.foo?.bar?;
const { thisProperty } obj?.whatIfI?.also?.want;
const { thisOne } = obj?.and;
const { thisOtherOne } obj?.and?.asWellAs;

p.s. there are useful nesting exceptions like json-schema/swagger and mongodb/elasticsearch-like queries, but most userland data-structures should be discouraged from that behavior for sake of keeping UX-workflow complexities manageable.

@TylerBarnes
Copy link

@kaizhu256 I disagree that it's a workflow problem. I'm not building these deeply nested objects, they're API responses. The point is that the API should return a complex and very specific object so I don't have to make a bunch of separate requests.
The place I see this problem most often is with REST and GraphQL responses.

@kaizhu256
Copy link

given a typical JSON api-response like following:

{
    "data": [
        {
            "id": "0.0hk40wqfxlfn",
            "rank": 6,
            "tags": ["aa", "bb"],
            "text": "hello world"
        },
        {
            "id": "kpsbcj4ik5",
            "rank": 7,
            "tags": ["cc", "dd"],
            "text": "bye world"
        }
    ],
    "meta": {
        "error": null,
        "pageNext": "s4xiuwqze0p",
        "pageOffset": 2,
        "pagePrevious": "tsele4s2qvb",
        "results": 2,
        "statusCode": 200
    }
}

the correct way to mitigate undefined-sub-property access is to pass it through a [throwaway] normalizer-function:

function responseNormalizer (response) {
/*
 * this function will normalize the response
 * by setting defaults to missing properties
 */
    // normalize response.data
    response.data = response.data || [];
    // normalize response.data[].tags
    response.data.forEach(function (elem) {
        elem.tags = elem.tags || [];
    });
    // normalize response.meta
    response.meta = response.meta || {};
    return response;
}

if a simple, throwaway-normalizer like above is not feasible, then the json-response is likely over-engineered and will cause further integration-headaches down the road.

graphql is useless outside of facebook. the effort to properly operationalize graphql (without access to facebook's internal tooling and support), is always less cost-effective than setting up [transparent] frontend-server/database caches.

@ljharb
Copy link
Member

ljharb commented May 28, 2019

@kaizhu256 graphql is far from useless outside of facebook; we use it extensively at airbnb, and your absolute statements, again, just aren't true.

@kaizhu256
Copy link

kaizhu256 commented May 28, 2019

you (and others) could've saved yourselves alot of unnecessary ux-workflow transformations by allocating those resources to [transparent] frontend-facing caches instead -- nodejs is perfect for writing throwaway, JSON-response cache-systems.

@zfrisch
Copy link

zfrisch commented May 28, 2019

@rijnhard I would say the use case isn't for merging objects, it's for grabbing more than one deeply nested property from the same object.

For your second example this is why it would be useful:

const {
  foo?: {
    bar?: { baz }
  },
  whatIfI?: {
    also?: {
      want?: {
        thisProperty
      }
    }
  },
  and?: { 
   thisOne,
   asWellAs?: { thisOtherOne }
  }
} = obj;

This is an issue I run into in React frequently. An API returns a complex object and I need to write all kinds of checks around accessing deeply nested properties. It's messy looking and not nice to read.
TBH, The above example is still not that nice to read, but it's a lot nicer – and it's simpler to write.

There are cases where I would have to write 20+ lines of this:

const { baz } = obj?.foo?.bar?;
const { thisProperty } obj?.whatIfI?.also?.want;
const { thisOne } = obj?.and;
const { thisOtherOne } obj?.and?.asWellAs;

I appreciate that problem. As a matter of fact, I'm currently rewriting/reworking a system for my Employer that utilizes REST endpoints that can be variable in the information they return, even outside of normal errors.

I've been following this thread for a while. After a lot of consideration, and purely from an analytical and maintenance perspective, I do not agree with using the ? parameter in bulk destructuring.

Your example kind of solidified that for me. It seems incredibly convoluted and incredibly difficult to read, and I think that's probably true for any professional. Using the old cliche of coming back to your code at a later time - I feel like you would likely have difficulty comprehending something that messy, for lack of a better term.

I will say, as much as the development community on the whole seems to hate it, I love JavaScript for a lot of different reasons, so this isn't meant to be malicious and if you want to change my mind and argue the merits, feel free to(you might succeed). I sometimes misinterpret what people mean so I'll be open to admit that if that's the case.

There are a lot of things I'd like added that would be beneficial, and I think an optional chaining operator would be useful in code outside of destructuring.

I'm sure everyone hates when testing code devolves into a && a.b && a.b.c && a.b.c.d and I would be thrilled to have a concise way of avoiding that.

In my opinion though, in any code where you are destructuring it means that you know the object already. There shouldn't be any surprises in the model, and if you're writing conditionals based on if the object you're receiving(especially in REST) has specific keys because it might not, I think you might want to revisit where that object is coming from.

What I'm saying is that imo: If a property value could exist than the property key should exist, even if that value is undefined.

Approaching it this way, you don't need to write destructuring anywhere in your code, you could simply pass the object you're receiving through a mask of the standard data model you expect using assign.

If you disagree, I hope you'll respond, but it just seems like the problem that this would be solving, would be solving it in a way that seems overly messy compared to other easier and more maintainable solutions.

@kaizhu256
Copy link

efficient routing/dispatching of http-requests is a common ux-workflow painpoint, made worse by nested routes/queries.

for example, github's nested-apis:

# v3 nested rest
curl -X GET 'https://api.github.com/repos/octocat/Hello-World/pulls'

# v4 nested graphql
curl -X POST 'https://api.github.com/graphql' -d \
'query {
    repository(owner:"octocat", name:"Hello-World") {
    pullRequests(last: 10) {
      edges {
        node {
          number
          mergeable
        }
      }
    }
  }
}'

are inherently less-efficient to dispatch (and cache) than a [hypothetical] flat-api like

curl -X GET https://api.github.com/repos-pulls-get\
?owner=octocat\
&name=Hello-World\
&last=10\
&projection=%5B%22number%22%2C%22mergeable%22%5D

the latter can be efficiently routed/dispatched with simple dict-lookups:

var flatRoutingDict;
flatRoutingDict = {
    "/repos-pulls-get": function (req, res) {
    /*
     * this function will handle the api-request /repos-pulls-get
     */
        var flatQueryDict;
        flatQueryDict = require("url").parse(req.url, true).query;
        /*
        flatQueryDict = {
            "owner": "octocat",
            "name": "Hello-World",
            "last": 10,
            "projection": "[\"number\",\"mergeable\"]"
        };
        */
        ...
    },
    "/repos-pulls-delete": function (req, res) {...},
    "/repos-pulls-patch": function (req, res) {...},
    "/repos-pulls-put": function (req, res) {...}
}

@corysimmons
Copy link

Didn't read all this but my use case is wanting to destructure GraphQL responses from headless CMS (and other third party things out of my control)...

So I frequently:

const { loading, data: { locations: { cities } } } = await apolloClient.query()

if (!loading && cities) console.log(cities)
// cities not defined

...so i have to resort to just abandoning almost all destructuring...

const { loading, data } = await apolloClient.query()

if (!loading && data && data.locations && data.locations.cities) console.log(data.locations.cities)

I suppose with optional chaining I could do something like...

const { loading, data } = await apolloClient.query()
const cities = data?.locations?.cities

if (!loading && cities) console.log(cities)

...????

That's not too bad assuming that's how it works, but god it'd be cool if it was like:

const { loading, data?: { locations?: { cities } } } = await apolloClient.query()

if (!loading && cities) console.log(cities)

a && a.b && a.b.c && a.b.c.d && a.b.c.d.e 💩is, always has been, and always will, be the bane of my existence when I'm making real world stuff. 😭

@kaizhu256
Copy link

excessive variables is bad-design in a datapassing-focused language like javascript -- the more variables declared, the longer it takes a human to back-track and figure out where they originated.

ideal code should use variables sparingly:

/*
 * a single variable is sufficient here.
 * 
 * this code is easier to debug for a human,
 * because he/she spends less time backtracking/up-scrolling
 * to figure out where all the variables came from.
 */
var response = await apolloClient.query();
// normalize response with defaults for missing data
response.data = response.data || {};

if (!response.loading && response.data.cities) {
    console.log(response.data.cities);
}
// cities not defined

@ljharb
Copy link
Member

ljharb commented Jun 7, 2019

I have the opposite experience - the more intermediate variables, often the easier it is to understand what’s happening, especially when stepping through via the debugger.

@zfrisch
Copy link

zfrisch commented Jun 7, 2019

I disagree that you have to be as explicit as kaizhu256 suggests. It's not absurd to me that you would pull out the references you want and store them in separate variables. Debugging was a great point, but also I think it's easy enough to determine where properties are coming from based on clear declarations.

const cities = data?.locations?.cities

The above is easy enough to track. My concern is that this is not:

const { loading, data?: { locations?: { cities } } } = await apolloClient.query()

Keeping in mind that this is literally a search for 1 property inside of the data object, could you imagine how messy it would end up looking if we searched for 2 or more?

I know that's a perspective based question, but I can't imagine many would be happy about running into 2+ searches in a destructure while doing maintenance on someone else's code.

You could mitigate that by writing destructuring into separate functions, but that's exactly what you could decide to do now and without the headache of additional syntax.

You could use a tailored proxy object to shorten your requests, or a constructed object with assigned getters or a constructed object with method-based helpers.

I'm a big fan of the object mask that I mentioned in my previous comment, but even if you decided not to do that, optional chaining + destructuring with multiple searches is almost more unintelligible than this:

let {cities} = ({loading} = response).data.locations;

Though again, I am really confused on why you would not know if there is a cities property on your object? For asynchronous responses I can't wrap my head around a data model being a guess instead of a known structure.

@kaizhu256
Copy link

@ljharb, agree-to-disagree. when i'm step-debugging unfamiliar-code, my first thought is where-the-hell did all these variables originate?

here's an example of me tracking down variables in tinymce's loader-logic (i want to disable its fetch-logic and instead inline everything into two, self-contained, css and js rollups).

image

@corysimmons

This comment has been minimized.

@claudepache

This comment has been minimized.

@corysimmons

This comment has been minimized.

@TylerBarnes
Copy link

@zfrisch "Though again, I am really confused on why you would not know if there is a cities property on your object? For asynchronous responses I can't wrap my head around a data model being a guess instead of a known structure."

in my experience you wouldn't know that your data structure fully exists in the case of templating. A template can be used for potentially many data structures and in the case of having it hooked up to a CMS, the data may be dynamic and not all of it will always exist.

I would agree with @corysimmons that debugging minified code isn't a particularly good argument for not adding optional chaining to destructuring. @ljharb If you want to debug that more easily you should load the unminified script.

There's a lot of talk about how the data should be structured to begin with, but that's not practical in a lot of real world situations where the data response is out of our control for whatever reason. To me that actually seems to be one of the big reasons for having optional chaining at all. To make sure you can grab nested properties and return undefined instead of throwing an error when an intermediate property isn't set.

Sure, throw-away normalizers are a solution, but not a particularly enjoyable thing to debug when you're reading hundreds of normalizers someone else wrote, when optional chaining could do the same thing without the need for an extra level of abstraction.

Maybe I just find scanning nested curly brackets easier than some people but in all the examples posted so far, optional chaining with destructuring seems easier to read and less messy than rows of regular optional chaining. Having the destructured code broken onto multiple lines feels easier to scan since there's a clear hierarchy. Either way it doesn't seem like this is really the place for debating how the data structure should originally be, since that's outside of the scope of this functionality.
I guess maybe it's an agree to disagree kind of situation though.

@zfrisch
Copy link

zfrisch commented Jun 7, 2019

@TylerBarnes nice response. You're right when you say we shouldn't be discussing model paradigms, so we kind of get off track in that regard.

If we're talking purely about the optional destructuring syntax existing in the first place, if optional chaining gets implemented ( which I think we all agree would be great ) it does make sense to have it be similar to the current destructuring syntax. Whether or not people think it's a bit much to dig through, like I honestly do, just means they won't be using it in their own code.

It does concern me purely for the reason that I think that outside of base examples it can really damage maintainability and debugging.

Consider a really basic task model, for a ticket queue or something:

const {
  taskId,
  taskStatus,
  taskType,
  taskData: {
    vendor ? : {
      vendor_name,
      vendor_email,
      vendor_phone
    },
    customer ? : {
      contact ? : {
        email,
        phone,
        web ? : {
          facebook,
          twitter,
          instagram
        }
      }
    },
    info ? : {
      closed ? : {
        date,
        time,
        user
      },
      pending ? : {
        date,
        time,
        user
      },
      open ? : {
        date,
        time,
        user
      }
    }
  }
} = response.data;

please correct me if this is a wrong interpretation

Optional destructuring would give programmers the ability to deal with encapsulating all possible models into a single destructure, but let's say you had an issue client-side where one of the primitive values date, user, or time was causing an error.

Currently the smart thing would be to have different files denoting different structures based on taskType (a.e. task/type/open.js, task/type/closed.js, task/type/pending.js) - you could hunt down easily which type was causing the issue - but with optional destructuring you can( and so we assume you would ) throw the structure into one single file ( task/structure.js) . This leads to the same reason why with became considered bad practice and generally frowned upon- you don't know where the variables user, time, and date come from.

I think of the same problem when you talk about templating models being dynamic. My initial thought is that you wouldn't have a one-size-fits-all function that deals with every possible instance of returned data using control structures.

Why?

You're setting yourself up for a large, explicit codebase. When performing templating I would think there would be something analogous to a type property that would inform your code how to process the data instead of saying "get each possible property individually and then check if they exist".

It seems similar to the difference between a new programmer writing tic-tac-toe vs a veteran. (This is not meant as an insult in the slightest. Anyone following tc39 proposals clearly is far enough along to know what they're talking about :) - it's just an example) The new programmer writes the code and says "if box1 and box2 and box3 are x or box1 and box4 and box7 are x" etc. The veteran writes a two line bitmask.

@noah79
Copy link

noah79 commented Oct 2, 2019

Optional chaining is syntactical sugar, yes. You can get the same result as the snippet @armanozak posted:

const prop = ((foo || {}).bar || {}).baz;

But the optional chaining syntax is much more readable. That snippet is "ugly" and the more nested the uglier it gets because you have to start wrapping things in parentheses more and more.

So while default values in destructuring can get you the end result:

const { foo: { bar: { baz } = { } } = { } } = obj || { };

It's not as readable as optional destructure chaining could be:

const {
  foo?: {
    bar?: { baz }
  }
} = obj;

This is the obvious syntax and is greatly preferred over having to check undefineds all the way down the chain and splitting destructuring statements interlaced with if() checks.

That said, I'm super excited about typescript 3.7 adding ?. and ?? to the spec.

@noah79
Copy link

noah79 commented Oct 2, 2019

const cities = data?.locations?.cities

The above is easy enough to track. My concern is that this is not:

const { loading, data?: { locations?: { cities } } } = await apolloClient.query()

I strongly disagree that the 2nd statement isn't easy to track. It is incredibly common to need to destructure a very large number of properties from props, state, store state, etc when rendering components in react. Reading a destructure statement is easy IMO. It is an anti-pattern to do something like const foo = thing.foo, bar = thing.bar instead of const {foo, bar} = thing

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

No branches or pull requests