[Request for feedback] Add deep property access via dot notation

[captbaritone]

This pull request should be safe to merge, but it lacks support for escaping characters in path strings. In most cases this is fine, since the more explicit array syntax can be used as a fallback. However, in the case of _.matches and friends, there is no such fallback available.

Should we explore an escape syntax (similar to Lodash's) which allows for complex paths via a string syntax? If so, should it be solved as part of this pull request?

As part of this pull request we get _.get essentially for free.

[coveralls]

Coverage increased (+0.05%) to 96.809% when pulling 8eaa7d7 on captbaritone:dot-notation into 5b60fd4 on jashkenas:master.

[captbaritone]

Thoughts @jridgewell @jashkenas @megawac?

[jashkenas]

I don't love string paths in JavaScript, but this looks fine to me — without the fancy dotted-path escaping.

[megawac]

I like the proposal for array style paths as they are very explicit, but I don't like the concept of partial string parsing. I think if we support string paths we should be able to parse the complete javascript path ast.

I would prefer to have users can convert their string paths to array paths using a multitude of third party libraries or a simple approach as in this code using split. We can also consider leaving a hook so users can override the behaviour of the path parsing

P.S. this isn't too strong of an opinion, I just don't want this to be a can of worms of things to support in the future.

[jashkenas]

P.S. this isn't too strong of an opinion, I just don't want this to be a can of worms of things to support in the future.

Very much agreed.

I think if we support string paths we should be able to parse the complete javascript path ast.

What is the "complete javascript path ast"?

[megawac]

Didn't make that very clear, but complicated paths rules like a["b.c[ab]"] which parses as ['a']['\"b.c[ab]\"']

Here's a couple examples from underscore-contrib https://github.com/documentcloud/underscore-contrib/blob/master/test/object.selectors.js#L30-L109

[coveralls]

Coverage increased (+0.05%) to 96.809% when pulling b3252bb on captbaritone:dot-notation into 5b60fd4 on jashkenas:master.

[captbaritone]

I like @megawac's idea of allowing users to customize path parsing so I've added a commit which exposes our path parsing function as _.toPath. This name matches with lodash.toPath.

I think we probably should support complex paths. Although like both of you, I don't feel terribly strongly about it, especially given:

• For all methods except the matcher methods, users can fall-back to the explicit array syntax.
• Users can now write their own _.toPath if they really need/want it.

Maybe we can merge this PR as is and I'll follow-up with a PR that adds complex path parsing? Then we can decide if the performance/weight/complexity of the implementation is worth the added convenience?

[jashkenas]

I think that I disagree pretty strongly with exposing _.toPath as part of the public API.

That's the kind of fiddly detail that makes Underscore more difficult to understand, more difficult to use, and more difficult to share code / examples across different projects that override _.toPath in different ways.

If we're going to support string paths, we just just do the right thing. Maybe that's a simple path shortcut that's not supposed to handle all of the complex cases, maybe that's comprehensive path parsing — but we shouldn't be pushing the conceptual complexity on to the user.

[captbaritone]

I suppose there are really two things here:

1. Should we expose _.toPath as a utility function.
2. Should we allow users to customize the behavior of the internal _.toPath.

I don't see any reason not to do number 1. We already have it, and it may be useful to someone else.

As for number two, I think we did set some precedence for this type of customization in #2480 where we allowed users to supply their own custom _.iteratee.

I would agree with @jashkenas that customizing these methods is probably an anti-pattern. It makes your code behave differently than documented. Especially on a project with multiple engineers, customization would probably be a bad choice. We should not be in the position of encouraging people to make bad choices. Then again, we're all adults here, why not let people make that tradeoff for themselves?

It does serve as a nice pressure release for the maintainers. We cannot be all things to all people, and features like _.mixins (and Underscore-contrib) have allowed us to empower users to get the functionality they need without feeing pressured to add that complexity to the core.

I realize I'm getting a bit philosophical here, and not actually forming a helpful opinions, so how about this:

We add support for complex paths. I think we can all see the value in it. With that in place I don't see any rational need to customize _.toPath so we can avoid having to make the philosophical call of "should we empower people to make decisions that are probably ill advised?".

[jashkenas]

We add support for complex paths. I think we can all see the value in it. With that in place I don't see any rational need to customize _.toPath so we can avoid having to make the philosophical call of "should we empower people to make decisions that are probably ill advised?".

That's a fair proposal, but then we get into a different discussion: What exactly do the complex paths look like? What's the supporting parsing code required to handle them robustly?

Again, reiterating my mild opinion that string paths in JavaScript are distasteful, we should weigh the pros and cons of the three options:

• No string path support.
• Basic string path support with a basic implementation.
• Robust string path support with a more serious implementation.

[bjmiller]

I believe that there's an option between point 2 and point 3, where a reasonable subset of paths are supported, and anything more complicated can be done another way.

Personally, I only want dot paths, and brackets for arrays. So, a.b[0].c would work. That's 99⅝ of use cases, I think.

[jashkenas]

Personally, I only want dot paths, and brackets for arrays. So, a.b[0].c would work. That's 99⅝ of use cases, I think.

That complicates things.

Why do you need a.b[0].c, when you have: a.b.0.c?

[bjmiller]

Because this is how people think when descending their data structures.

Would you be amenable to this if I wrote a toPath implementation that handied it well?

[jashkenas]

Would you be amenable to this if I wrote a toPath implementation that handied it well?

Sure!

[captbaritone]

@bjmiller Could it be as simple as stripping out the surrounding []s with a regex and then doing split('.')?

[bjmiller]

Not that simple, but probably not an order of magnitude more complex, either.

[captbaritone]

@bjmiller Any timeline on when you would be able to get to this?

[bjmiller]

I've done some work on it, I'm hoping to finish up soon. Perhaps this weekend? Let me know if there's a date you're trying to get this by.

I think I'm going to put up a gist with my approach first, and I'll make a proper PR out of it with tests and all if it's not rejected out of hand.

[captbaritone]

No specific timeline, but I wanted to make sure we're maintaining some sort of momentum.

…

-- Jordan Eldredge (mobile)

On Jan 25, 2017, at 9:26 PM, Brian Miller ***@***.***> wrote: I've done some work on it, I'm hoping to finish up soon. Perhaps this weekend? Let me know if there's a date you're trying to get this by. I think I'm going to put up a gist with my approach first, and I'll make a proper PR out of it with tests and all if it's not rejected out of hand. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[bjmiller]

Here's my first draft, in a gist: https://gist.github.com/bjmiller/be6a391e1f766ffe7010d3384ef74a2e

Let me know what you think. If you all hate it, I'll go a different way.

[Dexmaster]

Maybe it can be simplified a bit, like "a.b[0].c".replace(/(^\[|\]$)/g,"").split(/\]?[\.\[]/). Which will decrease a need in anything else...
It will work for all cases as number 2 and all basic user needs.

Description of what it does:

.replace(/(^\[|\]$)/g,"") // remove first "[" and last "]" to have a clean array after split
.split(/\]?[\.\[]/) // split by "].",".","][","["

[jgonggrijp]

Fast-forward nearly four years, and it turns out we went with a customizable public _.toPath anway, except that it defaults to no string path support at all. See #2877 and documentcloud/underscore-contrib#231.

[Dexmaster]

@jgonggrijp I believe it does not fix this issue it just ignores a choice of deep property access via dot notation...

P.S. I like that it has _.get now, but I would prefer deepProperty access via string path.

[jgonggrijp]

@Dexmaster you can override _.toPath in order to enable string paths. Contrib has a good internal string path parsing function that will be made public in the future. Please read the tickets I referenced above.

[Dexmaster]

Contrib has a good internal string path parsing function

It's not the best but ok, and it's just _.omitPath, not _.toPath or _.get :) You can override a lot of functions, it won't become part of a library or make the library useful...

I'm not bashing underscore.js it's a good lib, but it can add some useful parts.

P.S. For comparison from 2016 I was using lodash, which was based on Underscore.js, but had much more useful _.get for times you need to get the property via dot notation you are not sure exists (there was no optional chaining/elvis?. operator then)

[Dexmaster]

It's easier to use another lib, than fix one (create overrides), for every project you need it in...

[jgonggrijp]

@Dexmaster

It's not the best but ok,

If you see a way to improve on it, please feel welcome to submit a PR.

and it's just _.omitPath, not _.toPath or _.get :) You can override a lot of functions, it won't become part of a library or make the library useful...

_.toPath by itself is only one function, but it is also a configuration point like _.iteratee and _.templateSettings. It is used everywhere throughout the library, so if you override it, all functions that work with paths change their behavior accordingly. You need to override only one function to have string paths in all of them. See the documentation. Underscore-contrib will follow this convention in a future update as well.

It's easier to use another lib, than fix one (create overrides), for every project you need it in...

If you feel that Underscore is broken, then by all means use another library. But consider the other side: if we were to change all functions that process paths to split dotted strings, then all code like the following will break:

_.property('underscorejs.org')({
    'underscorejs.org': '2020-11-24',
    'npmjs.com': '2020-11-25'
});

As I see it, giving people a choice between array paths and string paths is the best we can do. Given that only one of the options can be the default, I think array paths are the safer option; it is more general and it doesn't break any code.

[Dexmaster]

@jgonggrijp

_.property(['underscorejs.org'])({
    'underscorejs.org': '2020-11-24',
    'npmjs.com': '2020-11-25'
});

Works just fine...

[Dexmaster]

And I'm not saying it should be perfect, I just mentioned to add a really good feature, that people will use in 4 years is not hard, even if it will be same as in ommitPath split('.') and that's it...

This PR was just simple if string split('.'), I mentioned two years ago how it can be improved if you wanted square brackets too... but it still would have worked, can't tell you more. I said my thoughts on this subject.

[Dexmaster]

@jgonggrijp

a choice between array paths and string paths is the best we can do

No, it is not, as in other libraries, you can provide multiple choices for people to select. It's much easier to save path string on a server and just use as is in _.get function on client-side.

As I said it's 4 years old PR, and it's spilt milk for all I care, I understand it wasn't prioritized, I just wanted to tell you your arguments are not working here. I'm glad you want to find a reason to justify closing this PR, but all the ones you've provided are not working.

If you feel that Underscore is broken, then by all means use another library.

I'm not saying underscore is broken. I said that 4 years ago I had a need to get a value from a complex object that was optionally appearing in some users, and to get it via lodash was much easier because they had _.get function with dot notation string which I could provide in a constant/var.

I'm saying this feature was not hard to approve, I don't see a reason why it was not approved till now.

And the reason was not provided here... And one that was provided I gave a solution, but it was still rejected without concrete rime or reason...

P.S. I'm not the creator of this PR, I still don't like how it ended...

[jgonggrijp]

@Dexmaster I think you're missing my point, and I might be missing yours as well. I recognize that you feel strongly about this. If you like, we can take this to a private chat to ensure we fully understand each other.

Edit: I'm on Gitter if that works for you.