Skip to content

fix: ensure ref is present before using - #210

Merged
benjycui merged 2 commits into
react-component:masterfrom
STRML:check-ref
Feb 7, 2017
Merged

fix: ensure ref is present before using#210
benjycui merged 2 commits into
react-component:masterfrom
STRML:check-ref

Conversation

@STRML

Copy link
Copy Markdown
Contributor

It's possible for the ref to be undefined, especially when using async batching strategies.

For example:

TypeError: Cannot read property 'getBoundingClientRect' of null
at getSliderStart(node_modules/rc-slider/lib/createSlider.js:170:0)
at calcValueByPos(node_modules/rc-slider/lib/createSlider.js:196:0)
at onMove(node_modules/rc-slider/lib/Slider.js:111:0)
at callback(node_modules/rc-slider/lib/createSlider.js:120:0)
at batchedUpdates(ui/utils/ReactRAFBatchingStrategy.js:29:13)
at unstable_batchedUpdates(node_modules/react-dom/lib/ReactUpdates.js:97:0)
at call(node_modules/rc-util/lib/Dom/addEventListener.js:21:0)
at apply(node_modules/add-dom-event-listener/lib/index.js:17:0)
at apply(node_modules/raven-js/src/raven.js:278:0)

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.3%) to 65.116% when pulling 4530398 on STRML:check-ref into ea13b42 on react-component:master.

@benjycui

Copy link
Copy Markdown
Member

Could you add a unit test for this in https://github.com/react-component/slider/blob/master/tests/common.test.js ? THX.

@STRML

Copy link
Copy Markdown
ContributorAuthor

Updated - also noticed that these event handlers could remain in the DOM if the component is unmounted before mouseup/touchend. Added a componentWillUnmount and corresponding test.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.08%) to 66.474% when pulling 9aa17d6 on STRML:check-ref into ea13b42 on react-component:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.08%) to 66.474% when pulling 749a13a on STRML:check-ref into ea13b42 on react-component:master.

@benjycui
benjycui merged commit 53b2463 into react-component:masterFeb 7, 2017
@benjycui

Copy link
Copy Markdown
Member

6.0.2

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@STRML@coveralls@benjycui