Customize object names 2

[bnanchen]

Hi,
This PR is the continuation of #338.
I've forked from @chromey and tried to address the problem when the user changes a property but the name of the object does not automatically change.

[josdejong]

Thanks for picking this up again @bnanchen!

Some feedback:

1. I think your approach to have the logic to keep the object names updated in the _onChange event handler is a good solution 👍
2. At first sight it looks like getExpandedNodes does return either the node itself or its childs, but shouldn't it return both itself and its childs? Or am I over looking something?
3. There is currently some duplication in the code, it will probably help to create a method Node.prototype.updateObjectName and use that in both updateDom and _onChange. I think this method should contain the try/catch in case the callback throws an exception.
4. Instead of the global getExpandedNodes, it is probably easier to create a method Node.prototype.recursivelyUpdateObjectName, which simply calls updateObjectName on itself and if expanded on each of it's childs. What do you think?
5. I think this can quite easily work for both for arrays too, would be great if you could allow that too.
6. I would prefer to pass only an object with the path, and maybe information about whether this is an Object or and Array. But not expose the interal this.childs since that is all undocumented internals.
7. Can you double check your indentation?

[bnanchen]

At first sight it looks like getExpandedNodes does return either the node itself or its childs, but shouldn't it return both itself and its childs? Or am I over looking something?

I've abandoned getExpandedNodes method, your proposition of Node.prototype.recursivelyUpdateObjectName is better and neater.

There is currently some duplication in the code, it will probably help to create a method Node.prototype.updateObjectName and use that in both updateDom and _onChange. I think this method should contain the try/catch in case the callback throws an exception.

I've implemented it in the commit f9a372a.

Instead of the global getExpandedNodes, it is probably easier to create a method Node.prototype.recursivelyUpdateObjectName, which simply calls updateObjectName on itself and if expanded on each of it's childs. What do you think?

I have implemented it in the commit 3df06e8.

I think this can quite easily work for both for arrays too, would be great if you could allow that too.

Can you be more explicit about this proposition?

I would prefer to pass only an object with the path, and maybe information about whether this is an Object or and Array. But not expose the interal this.childs since that is all undocumented internals.

The reason I picked up this issue is to have instead of the count a specific field as the object name. Therefore I found really useful to have access directly to the children of a node in the callback. I guess I'm not the only one that wants to use the object name to make a summary of what the subnodes are about.

[josdejong]

Thanks for the updates, I like your latest changes!

Can you be more explicit about this proposition?

I mean that right now you can customize the name of objects (customize object {5}). But similarly, we could allow customizing the name of arrays (customize array [12]).

The reason I picked up this issue is to have instead of the count a specific field as the object name. Therefore I found really useful to have access directly to the children of a node in the callback. I guess I'm not the only one that wants to use the object name to make a summary of what the subnodes are about.

That's a good point, I think you're right and this is a very common use case. How about passing the child count as property alongside the path then? Name it size or something. If you really need the child contents itself, you can always get the JSON contents using lodash get on the JSON object at hand so I don't think it's necessary to pass the childs themselves.

[bnanchen]

Hi,

I mean that right now you can customize the name of objects (customize object {5}). But similarly, we could allow customizing the name of arrays (customize array [12]).

Yes, that could be great. However, what should we provide to the callback? Also the path and the size?

That's a good point, I think you're right and this is a very common use case. How about passing the child count as property alongside the path then? Name it size or something. If you really need the child contents itself, you can always get the JSON contents using lodash get on the JSON object at hand so I don't think it's necessary to pass the childs themselves.

I was not aware of this. I'll modify the code accordingly.

[josdejong]

Thanks for your last changes @bnanchen 👍

Yes, that could be great. However, what should we provide to the callback? Also the path and the size?

I think the only change needed is to call onObjectName for both objects and arrays, and pass a new, third property type: 'object' | 'array' besides alongside the current properties path and size. Does that make sense?

[josdejong]

Thanks @bnanchen, and sorry for the late reply.

I really like it. I still feel there is some unnecessary repetition in updateNodeName. If I'm not mistaken the only difference between the object and array cases is {...} vs [...] right? So maybe we can do something like

if (this.type === 'object' || this.type === 'array') {
  // ...
  this.dom.value.innerHTML = (this.type === 'object')
      ? ('{' + (objName || count) + '}')
      : ('[' + (objName || count) + ']')
}

What do you think?

[bnanchen]

Yes, of course. I'd maybe written a little bit fast; I should have simplified the code from the beginning.
I applied the changes and I also have modified the callback name toonNodeName.
What do you think?

[josdejong]

Looks nice and clean, thanks Bastian 👍

[josdejong]

I will update the docs and do a release this weekend.

[josdejong]

I've updated the docs and did some more small changes (see commits above).

onNodeName is now available in v5.27.0. Thanks again :)

[bnanchen]

Glad to have contributed to this project :)