glittershark / reactable

Fast, flexible, and simple data tables in React
glittershark.github.io/reactable
MIT License
1.51k stars 222 forks source link

Only one table works per page #69

Open billschaller opened 10 years ago

billschaller commented 10 years ago

The defaultProps object that is used in Reactable is getting polluted with the first instance of column data created. From the React docs:

getDefaultProps
    object getDefaultProps()
Invoked once and cached when the class is created. Values in the mapping will be set on 
this.props if that prop is not specified by the parent component (i.e. using an in check).

This method is invoked before any instances are created and thus cannot rely on this.props. In 
addition, be aware that any complex objects returned by getDefaultProps() will be shared across 
instances, not copied.

Because of the way columns are passed around and modified, the default props object is getting polluted with data from instances of the class. This causes the first instance of a table to work fine, but the subsequent ones all have the same columns as the first, so if they use different columns, nothing displays.

To demonstrate, navigate to http://glittershark.github.io/reactable/ and run the following code in the console:

$('body').append('<div id="testTable"></div>');
React.renderComponent(Reactable.Table({data:[{foo:'bar', baz:'qux'}]}), document.getElementById('testTable'));

I don't have time to write a good fix for this at the moment.

RnbWd commented 10 years ago

I've also run into this issue. Defining the 'columns' prop in every component seems to fix this issue. Not ideal but it works.

glittershark commented 10 years ago

Yep. I'll see what I can do.

billschaller commented 10 years ago

The table should work using a single source of state -- right now props (and state) are being polluted -- it works against the basic principles of the library...

It's complicated of course, but the library should at a minimum:

joneshf commented 10 years ago

Just checked, and this isn't actually fixed it seems. You can verify by changing this line https://github.com/glittershark/reactable/blob/master/tests/reactable_test.jsx#L1957 to this.testNode1.find... and seeing that the tests still pass, when they should in fact fail.

In my case it still keeps the old header names. Can you explicate what is shared/why?

billschaller commented 10 years ago

Its the default props- the react docs do specify that they're shared among all instances of the component.

joneshf commented 10 years ago

I think I might have unclear. The issue is still there. The tests just aren't catching the actual issue. At least they weren't as of commit https://github.com/glittershark/reactable/commit/3cd0b972a28309e341c2a9e627637f5a0b1db80e Since it doesn't seem like the logic has changed since then, I think this is still not fixed. I can verify when I get to a computer though.

glittershark commented 10 years ago

@joneshf did you ever verify that this wasn't fixed?

joneshf commented 10 years ago

Yes, this still appears to be broken.

joneshf commented 10 years ago

Also, FWIW, @RnbWd 's suggestion of defining columns foreach table seems to allow multiple tables to work. Maybe that's got something to do with it.

glittershark commented 10 years ago

Cool, that's definitely helpful, thanks.