Do not crash when unsetting scrollToRow #62

IanVS · 2016-10-10T18:03:06Z

Description

This avoids trying to scroll to an undefined row.

Motivation and Context

Since the latest release (0.7.5), I began to see the following error:

Uncaught Error: Invariant Violation: Index out of range undefined

I found that this happened after setting a scrollToRow prop and then returning it to undefined.

How Has This Been Tested?

I tested this in my app, as well as added a test for it. Note: I needed to be able to setState, which required full rendering, so I add jsdom and react-dom to the dev dependencies.

Screenshots (if appropriate):

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.

KamranAsif · 2016-10-10T20:05:42Z

src/FixedDataTable.js

@@ -912,7 +912,7 @@ var FixedDataTable = React.createClass({
    }

    var lastScrollToRow  = oldState ? oldState.scrollToRow : undefined;
-    if (props.scrollToRow !== lastScrollToRow) {
+    if (typeof props.scrollToRow !== 'undefined' && props.scrollToRow !== null && props.scrollToRow !== lastScrollToRow) {


Maybe we should introduce lodash into our lib and use _.isNil instead?
We wouldn't need to worry about bloat if we implemented tree shaking.

That would be cool, but I guess I would argue that could be a separate PR after this one (if this is accepted).

KamranAsif · 2016-10-10T20:07:07Z

src/FixedDataTable.js

@@ -912,7 +912,7 @@ var FixedDataTable = React.createClass({
    }

    var lastScrollToRow  = oldState ? oldState.scrollToRow : undefined;
-    if (props.scrollToRow !== lastScrollToRow) {
+    if (typeof props.scrollToRow !== 'undefined' && props.scrollToRow !== null && props.scrollToRow !== lastScrollToRow) {


I think we need to implement something similar for the other: scrollTop, scrollToRow and scrollToColumn

I thought so too, but they weren't failing similar tests, while this one was, so I opted not to.

KamranAsif · 2016-10-11T13:55:12Z

build_helpers/test-globals.js

@@ -0,0 +1,28 @@
+'use strict'
+
+var jsdom = require('jsdom')


nit: missing semicolons throughout this file

That's what I get for copying it from somewhere else. Sure would be nice to have ESLint set up for this project. Any interest in a PR doing so?

Yeah, would love it!

IanVS · 2016-10-11T14:27:57Z

What will it take to get this merged? Add semicolons? Would you like checks around scrollTop, scrollLeft, etc?

KamranAsif · 2016-10-11T14:50:38Z

@IanVS Just want to checkout the branch and test a couple edge cases. Are you in a hurry to get this merged in?

IanVS · 2016-10-11T15:03:30Z

Nope, no particular hurry, just want to make sure I understand the expectations. Wasn't sure if you were done reviewing.

KamranAsif · 2016-10-11T16:48:58Z

Added some extra tests; found scrollTop was also failing.
I refactored the tests to reuse your instance and added some improvements to make it more generic

@wcjordan Can you give my changes a look?

wcjordan · 2016-10-11T16:51:33Z

src/FixedDataTable.js

@@ -920,7 +920,7 @@ var FixedDataTable = React.createClass({
    }

    var lastScrollTop = oldState ? oldState.scrollTop : undefined;
-    if (props.scrollTop !== lastScrollTop) {
+    if (typeof props.scrollTop !== 'undefined' && props.scrollTop !== null && props.scrollTop !== lastScrollTop) {


why use typeof for this check? why not just props.scrollTop !== undefined?

Not sure, I just copied it. I think we can just do props.scrollTop != null since undefined == null
Will update this and above

It's a habit I suppose, but it's usually considered safer. In strict mode accessing a variable that is undefined will throw a referenceError. That may not apply in this case, but as a general rule to be safe that's how I like to check for undefined variables.

Hmm that isn't my finding here: http://jsfiddle.net/s14sav6k/4/
But I'm not too familiar with strict mode. Do you have documentation which describes what can cause the ReferenceErrors? Could it be a browser specific implementation?

I hate pointing to SO as a source of truth, but here's this: http://stackoverflow.com/questions/2703102/typeof-undefined-vs-null

You'll get that error for variable reference, but not for property reference.
e.g. props.foo won't throw that error, as long as props is defined

Cool, I think the case you're referring to is specific to references on the execution context (props for this case), rather than accessed on an object through dot notation (e.g. props.scrollTop).

If there's a risk of props not existing, we'd need to also add a typeof props !== undefined && check to avoid an error from the dot access. If not, I think we're safe with the short hand Kam suggests in both strict and non-strict modes.

As I said, just a habit. ;)

wcjordan · 2016-10-11T16:53:20Z

src/FixedDataTableRoot-test.js

+     * @return {!Object}
+     */
+    getTableState() {
+      return this.refs['table'].state;


what's the motivation for this state access indirection? might be worth adding a comment.

We use it to inspect the inner state for the tests. I'll update the annotation to further clarify

wcjordan · 2016-10-11T16:53:34Z

I added a few questions, but lgtm

IanVS · 2016-10-11T17:12:15Z

Thanks everyone.

KamranAsif · 2016-10-13T18:06:32Z

Published in 0.7.6

Do not crash when unsetting scrollToRow

634de7d

KamranAsif reviewed Oct 10, 2016

View reviewed changes

KamranAsif self-assigned this Oct 10, 2016

KamranAsif reviewed Oct 11, 2016

View reviewed changes

Kamran Asif added 2 commits October 11, 2016 11:21

Adding semicolons

c7f5330

Adding more tests and fixing for scrollTop

d6be9c8

wcjordan reviewed Oct 11, 2016

View reviewed changes

CR feedback

4e36c9c

KamranAsif merged commit 79db986 into schrodinger:master Oct 11, 2016

IanVS deleted the fix-scrollToRow branch October 11, 2016 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not crash when unsetting scrollToRow #62

Do not crash when unsetting scrollToRow #62

IanVS commented Oct 10, 2016

KamranAsif Oct 10, 2016

IanVS Oct 10, 2016

KamranAsif Oct 10, 2016

IanVS Oct 10, 2016

KamranAsif Oct 11, 2016

IanVS Oct 11, 2016

KamranAsif Oct 11, 2016

IanVS commented Oct 11, 2016

KamranAsif commented Oct 11, 2016

IanVS commented Oct 11, 2016

KamranAsif commented Oct 11, 2016

wcjordan Oct 11, 2016

KamranAsif Oct 11, 2016

IanVS Oct 11, 2016

wcjordan Oct 11, 2016

IanVS Oct 11, 2016 •

edited

Loading

KamranAsif Oct 11, 2016

wcjordan Oct 11, 2016 •

edited

Loading

IanVS Oct 11, 2016

wcjordan Oct 11, 2016

KamranAsif Oct 11, 2016

wcjordan commented Oct 11, 2016

IanVS commented Oct 11, 2016

KamranAsif commented Oct 13, 2016

Do not crash when unsetting scrollToRow #62

Do not crash when unsetting scrollToRow #62

Conversation

IanVS commented Oct 10, 2016

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS commented Oct 11, 2016

KamranAsif commented Oct 11, 2016

IanVS commented Oct 11, 2016

KamranAsif commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcjordan Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcjordan commented Oct 11, 2016

IanVS commented Oct 11, 2016

KamranAsif commented Oct 13, 2016

IanVS Oct 11, 2016 •

edited

Loading

wcjordan Oct 11, 2016 •

edited

Loading