Add 'forEachNode' function

[IvanGoncharov]

Implemented as discussed in #41
@dchester @kristianmandrup What do you think?

[kristianmandrup]

Looks great! Would love to see it used in combination with my json-operator

[kristianmandrup]

I'm also curious if this could be tweaked to be used for querying/operating on an AST such as esprima or acorn. Would be amazing way to do code analysis or refactoring!!!

Of course it would not be json paths, but there are AST css like selector libs such as esquery and grasp :)

[IvanGoncharov]

@dchester Can you please review it?
I want to get confirmation before starting to work on docs & tests.

[dchester]

I will hope to take a closer look this evening, but quick thoughts from a glance:

• I worry a bit about having two interfaces for users -- one where jp.nodes returns plain objects with path and value properties, and a second where jp.forEachNode deals in _NodeWrapper objects. Let's ponder our options to make that more consistent...

• If we do go with something like this, I wonder if we can simplify to eliminate skip? Could we just support parent as its own special case and get rid of all that parameter passing? Or am I missing something?

[IvanGoncharov]

@dchester Thanks for the feedback.

I worry a bit about having two interfaces for users -- one where jp.nodes returns plain objects with path and value properties, and a second where jp.forEachNode deals in _NodeWrapper objects. Let's ponder our options to make that more consistent...

It depends on your standpoint on introducing breaking changes.
I can convert jp.nodes to return class Node with public path/value attributes instead of functions. Which in theory should work nicely with existing clients.

If we do go with something like this, I wonder if we can simplify to eliminate skip? Could we just support parent as its own special case and get rid of all that parameter passing? Or am I missing something?

In my use case, I frequently need to remove/update parent node and sometimes parent of parents.
One option would be to make parent return _NodeWrapper, so code will look like this:

node.parent().update(null); //replace parent with null
node.parent(1).remove(); //remove parent of parent

I can see only donwside is that we need to create new _NodeWrapper object and copy of path array using slice. I don't think it's very relevant since it still be way faster than calling jp.value/jp.parent.

[kristianmandrup]

I like this proposal, perhaps even make it configurable via factory function as an option? I tend to do that quite a lot...

"I can convert jp.nodes to return class Node with public path/value attributes instead of functions"

[IvanGoncharov]

@dchester I implement suggestions from my last comment as separate commit.
Can you please review them?

[IvanGoncharov]

@dchester Can you please review my last commit: b044def?
I want to know if I moving in the right direction before investing more time.

[dchester]

Sorry for the delay -- I'm travelling now but will have a chance to take a look tomorrow evening.

[kristianmandrup]

sure

[dchester]

So, after digging in here, I'm wondering if we should just stick with a procedural interface across the board.

I do like how the object-oriented approach feels specifically for the use case of deleting nodes, but I think it may be more complicated than we want/need to have the two different paradigms co-existing, and I'd rather keep backwards compatibility.

Given that, what do you think about an interface that would work something like this?

Delete a single node, given a path:

jp.delete(obj, '$.authors[0]') 

And a helper to iterate through all nodes and delete the ones where the supplied callback evaluates to true.

jp.prune(obj, value => value.name == 'rogers');

Or alternatively, for more control, for example to delete the parent node,

jp.nodes(obj, '$..*').forEach(n => {
  if (n.value.name == 'rogers') jp.delete(obj, jp.up(path));
});

In some ways this is less elegant and more verbose, but I like how it is consistent with the approach taken by the rest of the library. Does something in this direction solve the use-cases we're looking to solve?

[dchester]

Beginnings/explorations of the jp.delete and jp.prune ideas are here: https://github.com/dchester/jsonpath/compare/delete-prune

[kristianmandrup]

Sounds good to me :)

[IvanGoncharov]

@dchester IMHO jp.delete/jp.prune functions solve only symptoms, not the root cause. For example, it's a common task to unwrap some value (replace parent with value) does that mean jp.unwrap should be added? So this lib will quickly became specialize clone of lodash/underscore.

As for jp.up first obvious problem, it doesn't return you key so you still need to mess with path array to get it.

The second problem is that jp.up and jp.parent (not sure about jp.value) is useful almost exclusively to operate on path arrays returned from jp.nodes/jp.paths. So you have an array of keys which points to exact value but you still treat it like JSONPath which can point zero or more values. It's very confusing for the new user especially since it hard to guess that instead of a string you can pass an array with a mixture of strings and numbers.

To provide some background for my comments, my use-case involves the complicated transformation of JSON. Here is an example: https://github.com/APIs-guru/raml-to-swagger/blob/master/src/index.js#L409
And I don't see how I can improve code quality in such cases without wrapper object.
So if the solution is out of the scope of this library I will publish it as separate npm package (wrapper around jp.nodes).

[dchester]

Thanks for the example use case -- that helps. I see how the object-model approach simplifies those somewhat complicated transformations, where it would be useful to actually traverse up and down the data structure, etc.

That said, as I mentioned before, I am reluctant to mix paradigms and to expand the scope of this library, so I like your idea to have the Node wrapper functionality live as its own project (which, for what it's worth, I'd be happy to help with).

Good point also, about the array of path components being a potentially confusing interface for those helper methods. We should be able to make those methods accept strings as an alternative to path components, which be less surprising, at least.

[IvanGoncharov]

Closing stale PR.