15.5 support #147

KamranAsif · 2017-04-12T15:28:07Z

Description

Adding support for React 15.5
I used the codemods to add support
I couldn't get the site to run though; I kept seeing this error:

> fixed-data-table-2@0.7.14 site-dev-server /Users/Heaven/Work/fixed-data-table-2
> ./build_helpers/startSiteDevServer.sh

/Users/Heaven/Work/fixed-data-table-2/node_modules/react-docgen/dist/parse.js:60
    throw new Error(ERROR_MISSING_DEFINITION);
    ^

Error: No suitable component definition found.

@wcjordan could you check out this branch and give it a try?

Motiviations

#145

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

wcjordan · 2017-04-14T22:46:43Z

@KamranAsif this is because react-docgen doesn't yet support create-react-class. I've opened an issue. In the meantime you can change the package.json entry for react-docgen to point to "git://github.com/wcjordan/react-docgen#create_react_class_support_dist" and it should work. It uncovers some other errors though.

KamranAsif · 2017-04-15T02:19:16Z

src/FixedDataTableBufferedRows.js

@@ -101,7 +101,7 @@ var FixedDataTableBufferedRows = createReactClass({
  },

  _updateBuffer() {
-    if (this.isMounted()) {
+    if (this._rowBuffer) {


I don't know if this is a safe fix yet. The usage of isMounted scattered through seems scary

Hmm it seems fine to me, but ya removing isMounted wherever possible sounds good.

KamranAsif · 2017-04-15T02:20:21Z

Thanks for the fix @wcjordan!

Ran codemods on some more files. I'm trying to get the site to run without any errors.

KamranAsif · 2017-04-15T02:21:40Z

My next step is to remove the isMounted calls from FixedDataTable.js
https://github.com/schrodinger/fixed-data-table-2/blob/master/src/FixedDataTable.js#L1153

These seem a bit scary to me. I think we can fix a lot of these by removing the scroll handler when the component unmounts.

wcjordan · 2017-04-16T22:18:30Z

Sweet, this all looks good to me and removing unmount by removing the scroll handler sounds good. Consider this review lgtm'd and open a new one for the unmount change?

KamranAsif · 2017-04-30T14:44:01Z

@wcjordan I added this isMounted calls to this PR. You can view them seperately here: bbe16c8

bmv437

Hey @KamranAsif, I've tested your PR and it works! No more PropTypes or createClass warnings 🎉
Haven't noticed any issues. Please merge and release when you get a chance. Thanks for all your hard work 👍

wcjordan · 2017-05-02T14:40:19Z

src/FixedDataTable.js

      props.stopScrollPropagation
    );

    this.setState(this._calculateState(props));
  },

+  componentWillUnmount() {
+    this._wheelHandler = null;


Is there any risk of the onWheel still firing when we unmount? Or should we be okay since the DOM object is expected to be gone?

I don't think it will be an issue. React will automatically detach the scroll handler.

bmv437 · 2017-05-02T14:54:26Z

src/FixedDataTable.js

@@ -368,6 +368,7 @@ var FixedDataTable = createReactClass({
      this._onScroll,
      this._shouldHandleTouchX,
      this._shouldHandleTouchY


Needs a comma at the end of this line, that's why the build is failing on TravisCI.

Good catch, will fix

bmv437 · 2017-05-03T12:55:02Z

@KamranAsif Looks like Node v4 tests failed because it couldn't find the prop-types package. I think prop-types needs to get added to the devDependencies. That will probably fix the test.
Failed Test Log
package.json missing prop-types

KamranAsif · 2017-05-03T19:37:58Z

Yeah I saw the issue. Right now I'm just super busy onboarding at a new company so I haven't found time to wrap this up

bmv437 · 2017-05-03T20:50:35Z

No worries. Just trying to assist however I can. Thanks!

wcjordan · 2017-05-05T00:30:22Z

package.json

@@ -39,6 +39,7 @@
    "null-loader": "^0.1.0",
    "postcss": "^4.0.2",
    "postcss-custom-properties": "3.0.1",
+    "prop-types": "^15.5.8",


Should this & create-react-class be dependencies? React-redux has them as such
https://github.com/reactjs/react-redux/blob/master/package.json

Also update react-docgen version & ExampleImage to not use isMounted

wcjordan · 2017-05-10T04:39:40Z

@KamranAsif I updated the dependencies to be non-dev & I fixed a usage of isMounted in ExampleImage.
3b99f19

I think we should be ready to go if the tests pass.

KamranAsif · 2017-05-11T00:19:33Z

@wcjordan Actually..we don't list React or ReactDom as deps; instead as peer devs. Should these two be the same?

KamranAsif · 2017-05-11T00:19:49Z

I'm going merge this and push it out to unblock anyone

wcjordan · 2017-05-11T00:33:31Z

The issue with having them as peer deps is a React 14 or 15.1 user might be using the library, upgrade and then blow up cause they don't include prop-types & create-react-class. The peer deps will warn them... but still I think it's better to bring them in ourselves.

They're not big and I expect them to be stable so worst case we bundle a couple versions of them, it's not the end of the world.

Also looking at and following react-redux's practice:
https://github.com/reactjs/react-redux/blob/master/package.json

15.5 support

7ffc086

Adding several fixes

d9c1f50

KamranAsif force-pushed the master branch from 5a99a68 to d9c1f50 Compare April 15, 2017 02:18

KamranAsif commented Apr 15, 2017

View reviewed changes

wcjordan mentioned this pull request Apr 15, 2017

Migrating from React.createClass facebookarchive/fixed-data-table#488

Closed

Removing isMounted calls

bbe16c8

bmv437 approved these changes May 1, 2017

View reviewed changes

Merge branch 'master' into master

fdadd7b

wcjordan approved these changes May 2, 2017

View reviewed changes

bmv437 reviewed May 2, 2017

View reviewed changes

Fixing merge conflict

6c5558d

Adding missing package

97c6e99

wcjordan reviewed May 5, 2017

View reviewed changes

Move prop-types & create-react-class to dependencies

3b99f19

Also update react-docgen version & ExampleImage to not use isMounted

KamranAsif merged commit 4527549 into schrodinger:master May 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

15.5 support #147

15.5 support #147

KamranAsif commented Apr 12, 2017 •

edited

wcjordan commented Apr 14, 2017

KamranAsif Apr 15, 2017

wcjordan Apr 16, 2017

KamranAsif commented Apr 15, 2017

KamranAsif commented Apr 15, 2017

wcjordan commented Apr 16, 2017

KamranAsif commented Apr 30, 2017

bmv437 left a comment

wcjordan May 2, 2017

KamranAsif May 2, 2017

bmv437 May 2, 2017

KamranAsif May 2, 2017

bmv437 commented May 3, 2017

KamranAsif commented May 3, 2017

bmv437 commented May 3, 2017

wcjordan May 5, 2017

wcjordan commented May 10, 2017

KamranAsif commented May 11, 2017

KamranAsif commented May 11, 2017

wcjordan commented May 11, 2017 •

edited

15.5 support #147

15.5 support #147

Conversation

KamranAsif commented Apr 12, 2017 • edited

Description

Motiviations

Types of changes

Checklist:

wcjordan commented Apr 14, 2017

KamranAsif Apr 15, 2017

Choose a reason for hiding this comment

wcjordan Apr 16, 2017

Choose a reason for hiding this comment

KamranAsif commented Apr 15, 2017

KamranAsif commented Apr 15, 2017

wcjordan commented Apr 16, 2017

KamranAsif commented Apr 30, 2017

bmv437 left a comment

Choose a reason for hiding this comment

wcjordan May 2, 2017

Choose a reason for hiding this comment

KamranAsif May 2, 2017

Choose a reason for hiding this comment

bmv437 May 2, 2017

Choose a reason for hiding this comment

KamranAsif May 2, 2017

Choose a reason for hiding this comment

bmv437 commented May 3, 2017

KamranAsif commented May 3, 2017

bmv437 commented May 3, 2017

wcjordan May 5, 2017

Choose a reason for hiding this comment

wcjordan commented May 10, 2017

KamranAsif commented May 11, 2017

KamranAsif commented May 11, 2017

wcjordan commented May 11, 2017 • edited

KamranAsif commented Apr 12, 2017 •

edited

wcjordan commented May 11, 2017 •

edited