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

Does not handle empty string correctly #30

Closed
Tofandel opened this issue Apr 23, 2024 · 4 comments · Fixed by #11
Closed

Does not handle empty string correctly #30

Tofandel opened this issue Apr 23, 2024 · 4 comments · Fixed by #11

Comments

@Tofandel
Copy link
Contributor

Tofandel commented Apr 23, 2024

const fields = { foo: 'bar' };
console.log(getByPath(fields, '')); // undefined

setByPath(fields, '', 'baz');
console.log(fields); // { "": "baz", foo: "bar" }

Expected

const fields = { foo: 'bar' };
console.log(getByPath(fields, '')); // { foo: 'bar' }

setByPath(fields, '', 'baz'); // Throw exception path is empty
@saibotk
Copy link
Member

saibotk commented Apr 23, 2024

Ohhh interesting edge case, because "" is a valid property key.
Ouch, @djfhe what do you think about this cursed case?

We may need to talk about how we want to handle this case and what meaning the path parameter has.

But we should definitely add some test cases around those weird cases and adjust the behavior. Thanks for raising the issue!

@djfhe
Copy link
Member

djfhe commented May 20, 2024

Added some test cases for this, but it seems to work fine thou? 31
Sorry for getting to this so late.

@Tofandel
Copy link
Contributor Author

Tofandel commented May 20, 2024

Yes but that's not really the expected behavior is it?

I think an empty path should mean the current object, instead of a key with an empty string and the code seems to agree, https://github.com/clickbar/dot-diver/blob/main/src/index.ts#L286

But this line would never happen because array pop on a split string will always return a string, never undefined

@djfhe
Copy link
Member

djfhe commented May 21, 2024

I agree on that and will include the change as part of the next major release. Currently trying to optimize performance here, so that should (hopefully) not be to far off.

And yes, the check is useless and only exists to satisfy the typescript compiler, it will also get removed.

@djfhe djfhe mentioned this issue Jun 2, 2024
Merged
@djfhe djfhe closed this as completed in #11 Aug 18, 2024
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.

3 participants