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

Support array indexes #82

Merged
merged 14 commits into from
Jan 21, 2022
Merged

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Mar 4, 2021

Waiting for #80 before updating typings

Breaking change

Before:

dotProp.set({}, 'a.0', true);
//=> {a: {'0': true}}

After:

dotProp.set({}, 'a[0]', true);
//=> {a: [true]}

Fixes #71
Fixes #87

Signed-off-by: Richie Bendall <[email protected]>
@Richienb Richienb changed the title array-indexes Support setting array indexes Mar 4, 2021
@sindresorhus
Copy link
Owner

I think we should use a[0] as it's a more common syntax.

@Richienb
Copy link
Contributor Author

Richienb commented Apr 2, 2021

What should a.[0].b instead of a[0].b give?

@sindresorhus
Copy link
Owner

What should a.[0].b instead of a[0].b give?

IMHO, nothing. It would be the same as object['[0]']. We could maybe throw an error to let the user know they made a mistake.

@Zehir
Copy link

Zehir commented May 1, 2021

I think that a.0.b make more sense.
If a is an array he should return a[0].b.
If a is an object he should return a[0].bbecause you can't write a.0.b.
But for setting new values inside a non existing patch you can't detect if should be an array or an object.
And how you add an element inside a not existing array ? Like a[].b when a is currently undefined.
I don't think a lot of users are using [] as object keys but yes it's could be a breaking change.
And what about escaping the numbers like a.\\0.b. Or using a .. notation to say that the next value should be in an array.
An other question is if you set a value like a.5.b what you put inside the first 5 element of the array ? Undefined ?

To do this without breaking changes you could just not let the user create an new array with the dot notation but let them edit them, add items or get them with classic a.0.b notations. And if there is no number just add it to the array.

let data = {a:[]}
dotProp.set(data , 'a.0', 1);
console.log(data) // {a:[1]}
dotProp.set(data , 'a.1.b', 2);
console.log(data) // {a:[1, {b:2}]}
dotProp.set(data , 'a..b', 3);
console.log(data) // {a:[1, {b:2}, {b:3}]}
let data = {}
dotProp.set(data , 'a.0', 1); // Throw exception

Or with the .. notation:

let data = {}
dotProp.set(data , 'a..0', 1);
console.log(data) // {a:[1]}
dotProp.set(data , 'a..1.b', 2);
console.log(data) // {a:[1, {b:2}]}
data = {}
dotProp.set(data , 'a..1.b', 2);
console.log(data) // {a:[undefined, {b:2}]}

Currently the .. add an empty key but that make sence because there is no key between the two dots. So it's again a breaking change..

let data = {}
dotProp.set(data , 'a..b', 2);
{ a: { '': { b: 2 } } }

Edit: throw exception is not a good idea because this is a breaking change. Maybe you could just add or edit an array but not create it.

let data = {}
dotProp.set(data , 'a', []); // {a:[]}
dotProp.set(data , 'a.0', 1); // {a:[1]}
dotProp.set(data , 'a.2', 2); // {a:[1, undefined, 2]}
dotProp.set(data , 'a..', 3); // {a:[1, undefined, 2, 3]}
dotProp.set(data , 'a..b', 4); // {a:[1, undefined, 2, 3, {b:4}]}

@Richienb
Copy link
Contributor Author

@sindresorhus I've managed to turn the segmenting logic into a simple regex. Should I add it directly to the index.js file or does its scope warrant a seperate package?

@sindresorhus
Copy link
Owner

@Richienb Are you sure a regex is a good idea for this? I'm generally trying to avoid regexes unless absolutely necessary as it's so easy to accidentally make a ReDoS.

@Richienb
Copy link
Contributor Author

Good point. Regexes make it much easier to implement this extra feature but otherwise, we could just revert to scanning the string from left to right to segment it.

@sindresorhus
Copy link
Owner

Bump, in case you forgot about this. If you're just busy, feel free to ignore.

@Richienb
Copy link
Contributor Author

Richienb commented Sep 7, 2021

@sindresorhus Are you ok with the parsing strategy I mentioned above?

@sindresorhus
Copy link
Owner

we could just revert to scanning the string from left to right to segment it.

👍

index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
Signed-off-by: Richie Bendall <[email protected]>
Signed-off-by: Richie Bendall <[email protected]>
@Richienb Richienb marked this pull request as ready for review September 7, 2021 15:49
index.js Outdated Show resolved Hide resolved
@Richienb Richienb mentioned this pull request Sep 7, 2021
@Richienb
Copy link
Contributor Author

Richienb commented Sep 8, 2021

I have chosen to segment foo[bar] as ['foo', 'bar'].

Signed-off-by: Richie Bendall <[email protected]>
@Richienb
Copy link
Contributor Author

Richienb commented Sep 8, 2021

The tests are failing because of a bug: #86

@Richienb
Copy link
Contributor Author

Richienb commented Sep 8, 2021

CI is now passing.

test.js Show resolved Hide resolved
test.js Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I would like more tests with lots of different variations of invalid patterns (including different kind of escaping), just to ensure the parsing handles all edge-cases.

@sindresorhus
Copy link
Owner

I have chosen to segment foo[bar] as ['foo', 'bar'].

Why? I think that should be an invalid pattern and not return anything. It's beneficial to keep the syntax as simple as possible to reduce the chances of bugs and vulnerabilities.

@Richienb
Copy link
Contributor Author

Richienb commented Sep 9, 2021

I have chosen to segment foo[bar] as ['foo', 'bar'].

Why? I think that should be an invalid pattern and not return anything. It's beneficial to keep the syntax as simple as possible to reduce the chances of bugs and vulnerabilities.

I'm trying to be as lenient as possible on the errors so instead, I'll have this literally parsed as ['foo[bar]']

@sindresorhus
Copy link
Owner

Bump :)

@mikeerickson
Copy link

@Richienb @sindresorhus Is this PR working now so I can try it locally?

@Richienb
Copy link
Contributor Author

@mikeerickson It's mostly done but not completely.

@mikeerickson
Copy link

@mikeerickson It's mostly done but not completely.

ok, no problem. i will check back again in a week or so.

Signed-off-by: Richie Bendall <[email protected]>
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Bump => #82 (comment)

Happy new year :)

Signed-off-by: Richie Bendall <[email protected]>
@Richienb
Copy link
Contributor Author

I realised that an extremely lenient parser would probably lead to user confusion so I made it much stricter.

index.js Show resolved Hide resolved
@Richienb Richienb changed the title Support setting array indexes Support array indexes Jan 21, 2022
@sindresorhus sindresorhus merged commit d64e27b into sindresorhus:main Jan 21, 2022
@sindresorhus
Copy link
Owner

Great work, @Richienb 👍

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.

How to access data with array Support array index access
4 participants