agilgur5 / react-signature-canvas

A React wrapper component around signature_pad (in < 150 LoC). Unopinionated and heavily updated fork of react-signature-pad
https://agilgur5.github.io/react-signature-canvas/
Other
546 stars 119 forks source link

`TypeError: Cannot read properties of null (reading 'off')` during mount when running tests for a wrapper component #81

Closed AndrewOttavianoAbb closed 2 years ago

AndrewOttavianoAbb commented 2 years ago

Hello and thank you so much for your work on this project! I am creating a wrapper around the react-signature-canvas and am working on writing jest tests for it with testing-libarary/react. I am simply trying to mount it to ensure it displays properly and am receiving the following error. I'm not passing anything to it besides a ref (created with useRef); I'm not even touching canvasProps (at this point). I am rendering the parent <div /> with a data-testid so that I can grab hold of it in testing-library.

Test Code:

describe('Signature', () => {
  it('Component renders successfully', () => {
    render(<Signature SubmitComponent={() => <div />} />);
    const sigCanvas = screen.getByTestId('react-signature-canvas');

    expect(sigCanvas).toBeInTheDocument();
  });
});

(Signature is my wrapper component, this is what I'm actually rendering <SignatureCanvas ref={canvasRef} {...canvasAttributes} /> but canvasAttributes is null in my test above.)

Stack trace: ``` ● Signature › Component renders successfully TypeError: Cannot read properties of null (reading 'off') 5 | describe('Signature', () => { 6 | it('Component renders successfully', () => { > 7 | render(
} />); | ^ 8 | const sigCanvas = screen.getByTestId('react-signature-canvas'); 9 | 10 | expect(sigCanvas).toBeInTheDocument(); at t.r.off (../../node_modules/react-signature-canvas/build/index.js:1:3228) at t.value (../../node_modules/react-signature-canvas/build/index.js:1:3771) at callComponentWillUnmountWithTimer (../../node_modules/react-dom/cjs/react-dom.development.js:20413:14) at HTMLUnknownElement.callCallback (../../node_modules/react-dom/cjs/react-dom.development.js:3945:14) at HTMLUnknownElement.callTheUserObjectsOperation (../../node_modules/jsdom/lib/jsdom/living/generated/EventListener.js:26:30) at innerInvokeEventListeners (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:338:25) at invokeEventListeners (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:274:3) at HTMLUnknownElementImpl._dispatch (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:221:9) at HTMLUnknownElementImpl.dispatchEvent (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:94:17) at HTMLUnknownElement.dispatchEvent (../../node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:231:34) at Object.invokeGuardedCallbackDev (../../node_modules/react-dom/cjs/react-dom.development.js:3994:16) at invokeGuardedCallback (../../node_modules/react-dom/cjs/react-dom.development.js:4056:31) at safelyCallComponentWillUnmount (../../node_modules/react-dom/cjs/react-dom.development.js:20420:5) at commitUnmount (../../node_modules/react-dom/cjs/react-dom.development.js:20951:11) at commitNestedUnmounts (../../node_modules/react-dom/cjs/react-dom.development.js:21004:5) at unmountHostComponents (../../node_modules/react-dom/cjs/react-dom.development.js:21290:7) at commitDeletion (../../node_modules/react-dom/cjs/react-dom.development.js:21347:5) at commitMutationEffects (../../node_modules/react-dom/cjs/react-dom.development.js:23407:11) at HTMLUnknownElement.callCallback (../../node_modules/react-dom/cjs/react-dom.development.js:3945:14) at HTMLUnknownElement.callTheUserObjectsOperation (../../node_modules/jsdom/lib/jsdom/living/generated/EventListener.js:26:30) at innerInvokeEventListeners (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:338:25) at invokeEventListeners (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:274:3) at HTMLUnknownElementImpl._dispatch (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:221:9) at HTMLUnknownElementImpl.dispatchEvent (../../node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:94:17) at HTMLUnknownElement.dispatchEvent (../../node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:231:34) at Object.invokeGuardedCallbackDev (../../node_modules/react-dom/cjs/react-dom.development.js:3994:16) at invokeGuardedCallback (../../node_modules/react-dom/cjs/react-dom.development.js:4056:31) at commitRootImpl (../../node_modules/react-dom/cjs/react-dom.development.js:23121:9) at unstable_runWithPriority (../../node_modules/scheduler/cjs/scheduler.development.js:468:12) at runWithPriority$1 (../../node_modules/react-dom/cjs/react-dom.development.js:11276:10) at commitRoot (../../node_modules/react-dom/cjs/react-dom.development.js:22990:3) at performSyncWorkOnRoot (../../node_modules/react-dom/cjs/react-dom.development.js:22329:3) at ../../node_modules/react-dom/cjs/react-dom.development.js:11327:26 at unstable_runWithPriority (../../node_modules/scheduler/cjs/scheduler.development.js:468:12) at runWithPriority$1 (../../node_modules/react-dom/cjs/react-dom.development.js:11276:10) at flushSyncCallbackQueueImpl (../../node_modules/react-dom/cjs/react-dom.development.js:11322:9) at flushSyncCallbackQueue (../../node_modules/react-dom/cjs/react-dom.development.js:11309:3) at batchedUpdates$1 (../../node_modules/react-dom/cjs/react-dom.development.js:22387:7) at act (../../node_modules/react-dom/cjs/react-dom-test-utils.development.js:1042:14) at render (../../node_modules/@testing-library/react/dist/pure.js:97:26) at Object. (src/lib/signature/Signature.spec.tsx:7:5) at TestScheduler.scheduleTests (../../node_modules/@jest/core/build/TestScheduler.js:333:13) at runJest (../../node_modules/@jest/core/build/runJest.js:404:19) ```

Rendering in the browser seems to work. Any suggestions as to what I'm doing wrong or is this a bug?

agilgur5 commented 2 years ago

I am creating a wrapper around the react-signature-canvas and am working on writing jest tests for it with testing-libarary/react

Wrapper components are definitely encouraged since this is an unopinionated and unstyled component! Everyone's got their own UI components, UX flows, and CSS styles, so I think composition for a more specific use-case is good separation of concerns (while this component handles the generic use-case). When I originally built this component years ago, I also had an internal wrapper around it at my company, with buttons, a modal, react-bootstrap integration, etc. Also tested components great too 👍

Rendering in the browser seems to work. Any suggestions as to what I'm doing wrong or is this a bug?

Hard to tell without a reproduction as there's a number of moving pieces here. I've never seen this error before and a few pieces of your issue hint that this isn't a bug:

  1. The stack trace shows that is being hit during unmounting actually. I'm not sure why that's happening, but componentWillUnmount is being called. That calls the off method to remove the event listeners. In the current version of react-signature-canvas, the underlying signature_pad ref starts as null, so from the error message, it sounds like the internal ref is null during your mount. The next version of react-signature-canvas (the main branch) is written in TS and because of the strict typing, actually has an error check if the ref is null. It's a rare case, but it usually means you're calling a method during a mount/unmount (sounds related, right?). Even in that version though, it would still error, but it would give a more specific (and hopefully more helpful) error from react-signature-canvas itself.

  2. You mentioned that rendering in browser works, so it may very well be your test set-up. Internally, the tests I wrote for this component use jsdom and an additional window-resizeto polyfill (for mocking the resize functionality). If there's no DOM, a DOM ref wouldn't exist, so that could very well break a bunch of things like this. So I'm not sure what your jest.config.js looks like, but my suspicion would be that the test set-up is missing some configuration.

agilgur5 commented 2 years ago

Looking at the stack trace a bit more, jsdom does pop up so I'm assuming you are using jsdom? Also not sure which version of React this is on, as React 18 is currently untested (c.f. #76, which also discusses the need to migrate to RTL from Enzyme -- this may be related given that you're using RTL)

EDIT: Hmmm... it is possible you're running into the exact situation discussed in the TS rewrite review: https://github.com/agilgur5/react-signature-canvas/pull/42#discussion_r376819460 . As I mentioned there, it's possible that doing nothing (i.e. catching the error and doing a no-op instead) when the ref is null is more appropriate than throwing an error; the crux there was if such a situation were proper user behavior or erroneous behavior (where throwing an error is indeed appropriate). That rewrite is non-breaking though, as it would error out regardless. You're literally just mounting it, so err, that doesn't seem like erroneous behavior (😅 ), but as it only happens during testing, it still may be a testing set-up issue or a bug upstream in one of the testing libraries. Especially as this hasn't shown up in the many years this has existed until now

AndrewOttavianoAbb commented 2 years ago

@agilgur5 thank you for the quick response! I am using jsdom but it is the RTL wrapper around it import '@testing-library/jest-dom'; I am still on react 17, so that shouldn't be an issue.

agilgur5 commented 2 years ago

I am using jsdom but it is the RTL wrapper around it import '@testing-library/jest-dom';

Mmm @testing-library/jest-dom isn't a wrapper for jsdom, it's a set of DOM matchers for Jest. It doesn't depend on jsdom either.

A minimal reproduction would help here. Especially your jest.config.js, package.json, and your component's render code. The last part may be causing an error or something that's making it unmount in the testing environment.

AndrewOttavianoAbb commented 2 years ago

I am working on getting a repro ready to go. Unfortunately, the project I'm working on is closed source. I am trying to get a codesandbox example but am running into with fillStyle which I've read can be fixed w/jest-canvas-mock but am not getting anywhere with that.

Here it is if you want to give it a try: https://codesandbox.io/s/react-17-forked-y43gl3?file=/src/tests/Signature.spec.js

agilgur5 commented 2 years ago

I am trying to get a codesandbox example but am running into with fillStyle which I've read can be fixed w/jest-canvas-mock but am not getting anywhere with that.

Yea this is probably the crux of the issue -- the canvas implementation isn't working. react-signature-canvas's internal tests use node-canvas, which jsdom supports automatically if detected.

Here it is if you want to give it a try: https://codesandbox.io/s/react-17-forked-y43gl3?file=/src/tests/Signature.spec.js

You're using CRA for this CodeSandbox, so you may very well be running into https://github.com/hustcc/jest-canvas-mock/issues/72 . I couldn't get it to work in CodeSandbox, and that might be due to requiring native binaries like node-canvas etc, or due to CRA issues. Had some issues running in a node template (couldn't get things to install), but that may make more sense than the CRA template since Jest runs via Node etc.

I did create a more minimal StackBlitz project for this instead, and the tests pass fine there: https://stackblitz.com/edit/rsc-issue-81?file=index.spec.js . So I think the issue is within some part of your environment, as a minimal example succeeds.

AndrewOttavianoAbb commented 2 years ago

Switching to node-canvas fixed that test. Found the same suggestion here.

@agilgur5 thank you for your responsiveness and willingness to help!

agilgur5 commented 2 years ago

Fun fact, apparently I ran into the fillStyle issue as well when I was first creating the test suite 3 years ago in https://github.com/agilgur5/react-signature-canvas/issues/33#issuecomment-515737745 . Using compatible versions of node-canvas and jsdom resolved it there per that issue.