-
Notifications
You must be signed in to change notification settings - Fork 382
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
fix(jsonpath): tests and code for #646 and #651 #655
Conversation
src/record/json-path.js
Outdated
// see setValue fnc above for special handling of array item parsing vs numeric obj member name | ||
// e.g. 'object.1' parsing. this allows for support of parsing and differentiating object | ||
// member names that are also numeric values | ||
let str = this._path.replace(/\s/g, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is super expensive, doing three inline regexs and not even compiling them all upfront.
This is a pretty expensive solution.. I would need to look into this more first but I think it can possibly be done in a more performant manner. |
I'm guessing this is also a backwards compatability change to how people used the . vs [] This is certainly correctly behaviour, just a bit concerned about impact it may cause merging it in its current state. |
So discussed this with @yasserf a bit more. We don't want to do 3 regexes here
The first two should be compiled like the third one, but ideally we'd still only want to do one. A couple of solutions discussed to avoid the
This way inside the |
Closing this as its pretty expensive and needs to be addressed in a different manner. |
Courtesy of @honorcode, solves #646 and #651
Pretty happy with this, @yasserf do you want to give it a second pair of eyes.