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

Path should be an array #44

Closed
qwelias opened this issue Sep 9, 2019 · 8 comments · Fixed by #52
Closed

Path should be an array #44

qwelias opened this issue Sep 9, 2019 · 8 comments · Fixed by #52

Comments

@qwelias
Copy link

qwelias commented Sep 9, 2019

Path arg should be an array, since prop name could contain .

@kevva
Copy link

kevva commented Sep 10, 2019

Since using dot notation is the most popular way of describing objects, I'd rather return an escaped path like foo.foo\\.bar which corresponds to {foo: {'foo.bar': true}}.

@qwelias
Copy link
Author

qwelias commented Sep 10, 2019

@kevva
Popular where?
If in mongodb? Yes, because they prohibit dot as a key.
If in lodash? They allow for array for compatibility with dot in props.

And if you return "escaped" dots, how should consumer process the string? Will you include a function for unescaping too?

@kevva
Copy link

kevva commented Sep 10, 2019

Well, it's how you write it when accessing props on an object for example.

And if you return "escaped" dots, how should consumer process the string? Will you include a function for unescaping too?

You can access the property by using https://github.com/sindresorhus/dot-prop.

@qwelias
Copy link
Author

qwelias commented Sep 10, 2019

@kevva so would I have to install that package too to use this one? This is ridiculous

@kevva
Copy link

kevva commented Sep 10, 2019

No, it's just a suggestion. Using your proposed solution, you'd still have to convert the array to something before you could access the property of the object as well so I don't see what the tradeoff is here.

Also, the path is just a user friendly representation of the property that has changed. You don't have to install anything if you don't want to do anything with it, but it's not this modules purpose.

@qwelias
Copy link
Author

qwelias commented Sep 10, 2019

@kevva
The tradeoff is that if given

{
  'a.b':1,
  'a': { 'b': 2 },
}

Changing 1 or 2 in case of array would produce determined path (['a', 'b'] or ['a.b'] repectively), while using string would produce 'a.b' in both cases. And if you escape '.' with anything you'd have to split the string into valid array of props before unescaping where it was escaped as you suggested.

Also, considering, that implementation relies on dotted path in walkPath funciton there could be related bugs if props have dots in their names.

The point is, you cannot use dot as an object-tree path separator since it's a valid character for property names.

That's the reason popular utility libs use primarily array as object-tree path, string only as a convenience option. Look at lodash or ramda.

@kevva
Copy link

kevva commented Sep 10, 2019

I'm not saying that I disregard your solution, I just prefer the escaped one. It's non-breaking (most likely) and is directly compatible with dot-prop.

Also, considering, that implementation relies on dotted path in walkPath funciton there could be related bugs if props have dots in their names.

This is a non-issue IMO. It's easy to check for in code providing that the path is escaped. But yeah, I'm not opposed to returning path as an array either. Ultimately it's up to @sindresorhus to decide.

@qwelias
Copy link
Author

qwelias commented Sep 11, 2019

I think I'll start working on PR in spare time

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 a pull request may close this issue.

2 participants