Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError: Cannot read property 'offset' of undefined #830

Closed
fm9 opened this issue Sep 29, 2017 · 8 comments
Closed

TypeError: Cannot read property 'offset' of undefined #830

fm9 opened this issue Sep 29, 2017 · 8 comments

Comments

@fm9
Copy link

fm9 commented Sep 29, 2017

(This may have been fixed with the PR #820: passing -1 as the value for scrollToColumn&scrollToRow prevents the exception. Leaving it here for reference only.)

I am also seeing the exception reported in #297. It happens when I drag-resize the container enclosing a MultiGrid (which in turn resizes the MultiGrid). Here is some more detail (I am using [email protected]).

It all seems to start with this stack:

[0]    getSizeAndPositionOfCell           (CellSizeAndPositionManager.js:120)
[1]    getUpdatedOffsetForIndex           (CellSizeAndPositionManager.js:172)
[2]    getUpdatedOffsetForIndex           (ScalingCellSizeAndPositionManager.js:118)
[3]    _getCalculatedScrollLeft           (Grid.js:1034)
[4]    _updateScrollLeftForScrollToColumn (Grid.js:1049)
[5]    updateScrollIndexCallback          (Grid.js:497)
[6]    updateScrollIndexHelper            (updateScrollIndexHelper.js:42)
[7]    componentDidUpdate                 (Grid.js:484)
        .
        .
        .
--- stack bottom ---

The code executing in the top frame is:

return this._cellSizeAndPositionData[index]

index is the input parameter of getSizeAndPositionOfCell(index), and it gets here all the way from _getCalculatedScrollLeft() in frame [3], where it is called targetIndex and is calculated by:

var targetIndex = scrollToColumn < 0 ? finalColumn : Math.min(finalColumn, scrollToColumn);

This expression evaluates to NaN. It is NaN because scrollToColumn is NaN, which in turn is so because I do not pass it in as a prop to the Grid. If I do pass it in with, say, scrollToColumn={ 0 }, the exception disappears. However, if I do this and try to drag the Grid's scroll bar with the mouse, the Grid is continuously reset to position 0, which is no good. I need the scrollToColumn prop to stay undefined, but if I do that I get the error.

The actual exception is triggered when the NaN value returned above propagates into a variable called datum used at:

[0]    getUpdatedOffsetForIndex           (CellSizeAndPositionManager.js:173)
[1]    getUpdatedOffsetForIndex           (ScalingCellSizeAndPositionManager.js:118)
[2]    _getCalculatedScrollLeft           (Grid.js:1034)
[3]    _updateScrollLeftForScrollToColumn (Grid.js:1049)
[4]    updateScrollIndexCallback          (Grid.js:497)
[5]    updateScrollIndexHelper            (updateScrollIndexHelper.js:42)
[6]    componentDidUpdate                 (Grid.js:484)
        .
        .
        .
--- stack bottom ---

The offending line is:

var maxOffset = datum.offset

Since datum is NaN... boom.

All this also applies to scrollToRow, i.e., it must be defined to avoid the exception.

I haven't investigated why the exception doesn't occur in the demo at https://bvaughn.github.io/react-virtualized/#/components/Grid even when scrollToColumn is not defined. It may have to do with the fact that I am using MultiGrid. The MultiGrid demo doesn't accept undefined values for these props.

@fm9 fm9 changed the title TypeError: Cannot read property 'offset' of undefined (reopening issue #297) TypeError: Cannot read property 'offset' of undefined (related to issue #297) Sep 29, 2017
@fm9 fm9 changed the title TypeError: Cannot read property 'offset' of undefined (related to issue #297) TypeError: Cannot read property 'offset' of undefined (potentially related to issue #297) Sep 29, 2017
@fm9 fm9 changed the title TypeError: Cannot read property 'offset' of undefined (potentially related to issue #297) TypeError: Cannot read property 'offset' of undefined Sep 29, 2017
@zenyr
Copy link

zenyr commented Sep 30, 2017

I am also having this issue on react@16 (probably related to the scrollbar repositioning when column count decreases. not sure if this is still the case on react@15).

I'm temporarily using error-guarding (componentDidCatch) feature to prevent the whole SPA from breaking.

@bvaughn
Copy link
Owner

bvaughn commented Oct 14, 2017

I believe this should be fixed by PR #820 which was just merged and will be released with 9.11 in a few moments

@bvaughn bvaughn closed this as completed Oct 14, 2017
@Orangetronic
Copy link

Orangetronic commented Mar 21, 2018

We are bumping into what looks like this issue on 9.18.5
screenshot 2018-03-21 13 42 03

I'm not sure if the cause is going to turn out to be the same or not — me and @LawrenceHunt are currently doing some more detailed investigation and will update here. Hopefully we'll have some more conclusive info shortly!

Not sure if this is the best place for this, or if we should open a new issue

@aem
Copy link
Contributor

aem commented Mar 21, 2018

Is there a way to figure out which line of code is causing the error? There are several .offset calls in that class and it's hard to know which one is being called on an undefined reference. Providing a repro case would be helpful as well. Once you have some of that information and can confirm it's different than the issue above I'd open a new issue with more detail about your specific usecase

@Orangetronic
Copy link

Orangetronic commented Mar 21, 2018

It turns out we reimplemented this bug all by ourselves in a wrapping component and passed NaN in for scrollToRow. smdh

I'd be very happy to PR a defence + warning to cover that scenario if that's something you think would be good or helpful.

@aem
Copy link
Contributor

aem commented Mar 21, 2018

gonna defer to @bvaughn for the final decision but a warning covering the use case that someone doesn't pass a number to a prop expecting a number might be overkill. i imagine someone encountering the issue will find this discussion and be able to solve it themselves

@aem
Copy link
Contributor

aem commented Mar 21, 2018

Actually, on second thought, a PR (+ test case 😁) to cancel the action if a falsey value is provided, rather than throwing an exception and breaking down, and then manually log an error is probably a better UX. I'd say open the PR and we can have a discussion about it there!

@DarkKnight1992
Copy link

It turns out we reimplemented this bug all by ourselves in a wrapping component and passed NaN in for scrollToRow. smdh

I'd be very happy to PR a defence + warning to cover that scenario if that's something you think would be good or helpful.

thanks for pointing to the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants