feat: added onTreePopulated prop

[domagojk]

Needed for https://github.com/stoplightio/platform-internal/pull/7139

[mpodlasin]

@domagojk I've looked at this and linked PR but it's a bit hard for me to understand how necessary is this change.

Can you maybe elaborate a bit more what are you trying to achieve here? 🤔

It worries me, because those changes expose a lot of internals. I am sure it's warranted but maybe we can come up with a way to minimise the amount of added props. 🤷

[domagojk]

@mpodlasin This mirrors what has been done before which is that for each tree node, we are making a node in studio where we control its state (checked and required) so it can be send to the masking api.

I don't see a way of doing that without onTreePopulated because at that instance, tree is ready to be analysed. One small thing that I can do to expose a bit less internals is using (rootNode: RootNode) => void callback signature rather than (tree: JsonSchemaTree) => void.

As for event handlers, I'm using it to calculate the number of nodes in a tree because there is a limit on how big the model can be in order for it to be masked. But I have nothing against in doing that internally, maybe we can return that along with onTreePopulated callback, so it would be (rootNode: RootNode, nodeCount: number) => void and then I can remove treeWalkerEventEmitter property?

[mpodlasin]

@domagojk is there no way to retrieve the number of nodes from JsonSchemaTree directly? 🤔

[mpodlasin]

And yes onTreePopulated is the smaller of my worries, I would prefer to get rid of that event handler somehow :)

[mpodlasin]

I know that this useMemo is quite side-effect'y already, but we really shouldn't do that.

Simple refactor would allow for that function to be called in a separate useEffect hook.

[domagojk]

Can you elaborate more on what kind of refactor you have in mind?

[domagojk]

tracking isPopulated state and then using that in useEffect?

[domagojk]

oh I guess I could just use jsonSchemaTreeRoot as dependency, I will try it out

[mpodlasin]

useMemo can return the whole jsonSchemaTree.

Than you just make useEffect where that callback is called with jsonSchemaTree 🤷

[mpodlasin]

something like:

const jsonSchemaTree = React.useMemo(...);

React.useEffect(() => {
   onTreePopulated?.(jsonSchemaTree);
}, [jsonSchemaTree, onTreePopulated]);

[domagojk]

@domagojk is there no way to retrieve the number of nodes from JsonSchemaTree directly? 🤔

As far as i know, there is no way of doing that anymore. tree.count was available in previous JSV version, but it was removed

[mpodlasin]

@domagojk is there no way to retrieve the number of nodes from JsonSchemaTree directly? 🤔

As far as i know, there is no way of doing that anymore. tree.count was available in previous JSV version, but it was removed

I believe this would be much cleaner solution. But I don't want to block you.

Let's make the count a part of onTreePopulated, like you proposed, and we will be okay.

Just for future extensibility I would do something like

onTreePopulated={(tree, { nodeCount }) => ...}

Note how nodeCount is in an object - if in the future you will need something more, we can add it there.

[domagojk]

I think tree.count was removed because tree is now evaluated lazily so we don't know the number of nodes (it will change as you expand the tree). I'm not 100% tho, but I believe this is the case.

Thanks for the tip! 👍 I will try to minimise exposing so many things, but I just found a bug in the implementation so I will have to change this PR probably 🙄

[mpodlasin]

Yes! Looks great now! 🎉

[stoplight-bot]

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀