ankane / react-chartkick

Create beautiful JavaScript charts with one line of React
https://chartkick.com/react
MIT License
1.2k stars 58 forks source link

Fails jest tests #18

Closed MOZGIII closed 6 years ago

MOZGIII commented 6 years ago

I'm using a create-react-app based code base, and when I run tests I have the following errors:

Storyshots › Dashboard / ToDo List [Test] › standard

    TypeError: Cannot read property 'id' of null

      at createChart (node_modules/chartkick/chartkick.js:1751:30)
      at new BarChart (node_modules/chartkick/chartkick.js:1775:7)
      at ChartComponent.newChartType (node_modules/react-chartkick/dist/react-chartkick.js:141:7)
      at ChartComponent.componentDidMount (node_modules/react-chartkick/dist/react-chartkick.js:146:12)
      at commitLifeCycles (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:4553:24)
      at commitAllLifeCycles (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:5729:9)
      at HTMLUnknownElement.callCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1610:14)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:219:27)
      at HTMLUnknownElementImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:126:9)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:36:27)
      at HTMLUnknownElement.dispatchEvent (node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:61:35)
      at Object.invokeGuardedCallbackDev (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1649:16)
      at invokeGuardedCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1506:29)
      at commitRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:5833:9)
      at performWorkOnRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6800:42)
      at performWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6750:7)
      at requestWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6661:7)
      at scheduleWorkImpl (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6515:11)
      at scheduleWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6472:12)
      at scheduleTopLevelUpdate (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6976:5)
      at Object.updateContainer (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7014:7)
      at Object.create (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7637:18)
      at Object.renderOnly [as test] (node_modules/@storybook/addon-storyshots/dist/test-bodies.js:47:31)
      at Object.<anonymous> (node_modules/@storybook/addon-storyshots/dist/index.js:146:28)
          at new Promise (<anonymous>)
      at Promise.resolve.then.el (node_modules/p-map/index.js:46:16)
          at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

The issue is somewhere in chartkick.

ankane commented 6 years ago

Hey @MOZGIII, from the stack trace, it looks like render isn't being called for the component.

MOZGIII commented 6 years ago

Maybe it's just not in a stack trace? I'd expect commitLifeCycles to probably have called renders before the componentDidMounts (and having it return before the calls to componentDidMount).

ankane commented 6 years ago

If render is being called, then maybe ref isn't. https://github.com/ankane/react-chartkick/blob/395e2b0d1d89d699fe7333230961e581b3584bb2/index.js#L55

MOZGIII commented 6 years ago

I'm sure that ref is called as well. However, tests may do shallow rendering of some kind. Then it's possible for ref to be called with null.

On the other look, it's probably a variable scope leak error. https://github.com/ankane/react-chartkick/blob/395e2b0d1d89d699fe7333230961e581b3584bb2/index.js#L31 Here you call the function with this.element, but I think you need to create a local scope copy before doing that, like this:

var localElement = this.element;
new props.chartType(localElement, data, options)

This has to do with JavaScript's variable semantics.

ankane commented 6 years ago

If ref is called with null, it would explain the error. Not sure I follow the scope leak error theory.

MOZGIII commented 6 years ago

ref will also be called with null on unmounting. Regardless of the reason, checking that this.element is not falsy would fix the issue here.

Scope leak error is about variable being assigned the new value, and other code (typically a closure) that sees that variable in it's scope not being ready for it's value to change (assuming the closure has a local copy of the variable). Though, this is not the case here (my bad), because the code there is all synchronous and correct. Calling a function (line 31) implies assignment of it's arguments, and the code's not async, so nothing breaks before that - that means I'm wrong about the scope leak error.

I'd suggest adding a check for now. To debug the issue properly there has to be a jest testing setup made. Probably a good idea to have one anyway. Unfortunately I can' help with that right now, but it's pretty easy to set up.

MOZGIII commented 6 years ago

Thanks