Skip to content

Conversation

@agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Jun 15, 2022

Summary

Migrate testing framework from Enzyme to React Testing Library (RTL).
This paves the way for support for React 18 (#76), as it was not possible to test with Enzyme due to the lack of React 18 adapter.

Details

  • Enzyme only has unofficial, partial support for React 17 (Support for React 17 enzymejs/enzyme#2429) and no support what-so-ever for React 18 (Support for React 18 enzymejs/enzyme#2524), so forced to migrate (see also add React 17 to peerDep range -- needs to update devDeps and tests #64 (review) and React 18 support in peerDeps #76 (comment))

  • RTL is most certainly more of an integration testing library than a unit test library

    • can't access the instance through RTL, even though this component actually needs to use refs (bc canvas)
      • I managed to hack around this by using refs, but I honestly wasn't even sure that this was possible
        • notably hooks like useRef don't work since they can only be used inside of a React component or another hook
        • but callback refs and createRef do work fortunately
      • tried to use react-dom-instance initially, as suggested in a blog post by the author, but it only works with React 16 (see the issue I filed w/ repro: Doesn't work with React 17 or React 18 arqex/react-dom-instance#14)
        • in React 17 and React 18 it just returns false and doesn't get an instance
    • querySelector is an escape hatch that is not recommended, enough so that I read through the docs multiple times before finding it was even possible
      • was using document.querySelector directly before finding this
      • somewhat replaces Enzyme's exists
    • rerender is also a bit of an escape hatch
      • notably, you can't set props directly on a ref, .props is a read-only property, so had to find some alternative to be able to run the tests
      • replaces Enzyme's setProps
  • had to change clearOnResize to an optional type as it was required without this

    • despite that it's in defaultProps; the interface itself can't infer this, so probably makes sense to make it optional here as well

Next Steps

I tested this with React 18 as well and everything worked, so next PRs will add support for React 18 in peerDeps (#76)

- Enzyme only has unofficial support for React 17 and no support what-so-ever for React 18, so forced to migrate

- RTL is most certainly more of an integration testing library than a unit test library
  - can't access the instance through RTL, even though this component actually _needs_ to use refs (bc canvas)
    - I managed to hack around this by using refs, but I honestly wasn't even sure that this was possible
      - notably hooks like `useRef` don't work since they can only be used inside of a React component or another hook
      - but callback refs and `createRef` do work fortunately
    - tried to use `react-dom-instance` initially, as suggested in blog post by the author, but it only works with React 16
      - in React 17 and React 18 it just returns `false` and doesn't get an instance
  - `querySelector` is an escape hatch that is not recommended, enough so that I read through the docs multiple times before finding it was even possible
    - was using `document.querySelector` directly before finding this
    - somewhat replaces Enzyme's `exists`
  - `rerender` is also a bit of an escape hatch
    - notably, you can't set props directly on a `ref`, `.props` is a read-only property, so had to find some alternative to be able to run the tests
    - replaces Enzyme's `setProps`

- had to change `clearOnResize` to an optional type as it was required without this
  - despite that it's in `defaultProps`; the interface itself can't infer this, so probably makes sense to make it optional here as well
@agilgur5 agilgur5 added kind: internal Changes only affect the internals and not the API or usage scope: tests Tests could be improved. Or changes that only affect tests labels Jun 15, 2022
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #88 (08b4f1b) into main (cc6cce9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #88   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           75        75           
  Branches         9         9           
=========================================
  Hits            75        75           
Impacted Files Coverage Δ
src/index.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc6cce9...08b4f1b. Read the comment docs.

Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was a non-trivial refactor with lots of different issues, but the end code diff I achieved is actually relatively small! LGTM

@agilgur5 agilgur5 merged commit 8e431f5 into main Jun 15, 2022
@agilgur5 agilgur5 deleted the deps-migrate-rtl branch June 15, 2022 20:06
agilgur5 added a commit that referenced this pull request Jun 16, 2022
* deps: migrate from Enzyme to RTL

- Enzyme only has unofficial support for React 17 and no support what-so-ever for React 18, so forced to migrate

- RTL is most certainly more of an integration testing library than a unit test library
  - can't access the instance through RTL, even though this component actually _needs_ to use refs (bc canvas)
    - I managed to hack around this by using refs, but I honestly wasn't even sure that this was possible
      - notably hooks like `useRef` don't work since they can only be used inside of a React component or another hook
      - but callback refs and `createRef` do work fortunately
    - tried to use `react-dom-instance` initially, as suggested in blog post by the author, but it only works with React 16
      - in React 17 and React 18 it just returns `false` and doesn't get an instance
  - `querySelector` is an escape hatch that is not recommended, enough so that I read through the docs multiple times before finding it was even possible
    - was using `document.querySelector` directly before finding this
    - somewhat replaces Enzyme's `exists`
  - `rerender` is also a bit of an escape hatch
    - notably, you can't set props directly on a `ref`, `.props` is a read-only property, so had to find some alternative to be able to run the tests
    - replaces Enzyme's `setProps`

- had to change `clearOnResize` to an optional type as it was required without this
  - despite that it's in `defaultProps`; the interface itself can't infer this, so probably makes sense to make it optional here as well

* fix lint issues
Hamza65523 pushed a commit to Hamza65523/react-signature-canvas that referenced this pull request Nov 2, 2025
* deps: migrate from Enzyme to RTL

- Enzyme only has unofficial support for React 17 and no support what-so-ever for React 18, so forced to migrate

- RTL is most certainly more of an integration testing library than a unit test library
  - can't access the instance through RTL, even though this component actually _needs_ to use refs (bc canvas)
    - I managed to hack around this by using refs, but I honestly wasn't even sure that this was possible
      - notably hooks like `useRef` don't work since they can only be used inside of a React component or another hook
      - but callback refs and `createRef` do work fortunately
    - tried to use `react-dom-instance` initially, as suggested in blog post by the author, but it only works with React 16
      - in React 17 and React 18 it just returns `false` and doesn't get an instance
  - `querySelector` is an escape hatch that is not recommended, enough so that I read through the docs multiple times before finding it was even possible
    - was using `document.querySelector` directly before finding this
    - somewhat replaces Enzyme's `exists`
  - `rerender` is also a bit of an escape hatch
    - notably, you can't set props directly on a `ref`, `.props` is a read-only property, so had to find some alternative to be able to run the tests
    - replaces Enzyme's `setProps`

- had to change `clearOnResize` to an optional type as it was required without this
  - despite that it's in `defaultProps`; the interface itself can't infer this, so probably makes sense to make it optional here as well

* fix lint issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: internal Changes only affect the internals and not the API or usage scope: tests Tests could be improved. Or changes that only affect tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants