Can't change proptypes for node & rendered component about react-virtualized-tree HOT 8 CLOSED

Hi @Avivhdr.

Nice catch. There are a few points in here.

1. onChange is actually required for real, if it isn't supplied the expansion will throw an error, that onChange should be the one injected by your child function (https://github.com/diogofcunha/react-virtualized-tree//blob/master/demo/src/examples/LargeCollection.js#L33) - I'm waiting on react 17 to implement this with the new context API and kill off the need to inject this properties that are the real definition of context, it just looked quite messy with the old context api
2. onMeasure should be optional.
3. on the nodeShape I'm tempted to say that we should change it to be either nodeShape or object to make it as extensible as possible.

What do you think? Would you be able to open a PR addressing points 2 and 3? I would be eternally grateful 😺, otherwise I will probably do it over weekend.

Cheers.

from react-virtualized-tree.

Thanks for the fast reply @diogofcunha.
how would you recommend to do this change (No. 3) ?
Should the isRequired be removed from the name prop or should the Tree component receives another prop called nodeShape that will determine how the shape of the node should be, like this:

const nodeShape =  {
    groups: PropTypes.arrayOf(PropTypes.shape({
        id: PropTypes.number,
        name: PropTypes.string,
        state: PropTypes.shape({
            expendable: PropTypes.bool,
        }),
        children: PropTypes.arrayOf(PropTypes.shape({
            color: PropTypes,
            id: PropTypes.number,
            name: PropTypes.string,
            licensePlate: PropTypes.string,
            // {...more children props}
        })),
        // {...more group props}
    }),
}

......
      <Tree nodes={nodes} nodeMarginLeft={0}> nodeShape={nodeShape}
        {
          ({ node, ...restProps }) =>
            <Expandable node={node} onChange={this.handleChange}>
              {.......}
            </Expandable>
        }
      </Tree>

In short, instruct me and i'll do my best!

from react-virtualized-tree.

Hi @Avivhdr.

I don't think it's necessary to inject the nodeShape as a prop, specially because I will probably rewrite the library in typescript in the near future and it would be ignored anyway.

I was investigating a little bit an what's clear in my mind is that we have a required structure for a node, each is:

id: string | number;
state?: { };
children?: Node[]

It would be good to check that these are valid on runtime to avoid bugs, all the rest is not very relevant to the typechecking on the library side, at the user side people can do stricter typechecks without any problem.

The trick here is that PropTypes are just functions, so if we can make a checker that checks for the required properties and that doesn't throw warnings for extra props it would be amazing.

Of course that has the library also ships with ts definitions that also need to be changed.

I may need to get my hands in the code to figure this out a little bit better

from react-virtualized-tree.

On the other hand I wonder if not having any of these required properties shouldn't just throw an actual descriptive error as it compromises the good behaviour of an application and can prevent a lot of unexpected bugs.

If we just throw (and I know this would be a bold decision) we could in theory just say that node is a PropTypes.object, but let's try to make a proptype checker first

from react-virtualized-tree.

If we define Node as being a function for proptype checking proposes.

The way to define a custom validator is: (source)

function(props, propName, componentName) {
    if (!/matchme/.test(props[propName])) {
      return new Error(
        'Invalid prop `' + propName + '` supplied to' +
        ' `' + componentName + '`. Validation failed.'
      );
    }
  },

Do you think you can play around with it a little bit so that:

• An error is thrown when id is not supplied
• Works for children too
• Doesn't throw warnings for extra props

I can figure out the typescript part over the weekend

from react-virtualized-tree.

Hi @diogofcunha I'll be happy to try and contribute but first i need to have a clear view on what to do.
I don't undarstand the benefits of creating a custom validator function that will allow extra props and will throw when id is not passed when this can be achieved easily by removing the isRequired from the name prop here: https://github.com/diogofcunha/react-virtualized-tree/blob/master/src/shapes/nodeShapes.js#L11

from react-virtualized-tree.

hey @Avivhdr sorry about the delay I was on holiday.

Let's do that and I will try to figure out a way to have static typing for typescript

from react-virtualized-tree.

@diogofcunha
#45

from react-virtualized-tree.