Skip to content
Snippets Groups Projects

Make a bit more progress towards upgrading to React 18

Merged Axel Bocciarelli requested to merge towards-react-18 into main

This supersedes !450 (closed).

I spent the last couple of days trying, once again, to upgrade to React 18... but once again I can't get passed all the failing tests. I tried setting up Jest independently from Create React App in order to use the latest version; I tried installing all the latest version of the Testing Library packages; I tried refactoring various things in the app; ... I tried sooo many workarounds, but every time I change something, something else breaks for no clear reason. :disappointed:

There's definitely a combination of:

  • Jest/Testing Library/React struggling to handle asynchronicity in a JSDOM environment — one example I encountered is components rendered with React.lazy() getting stuck on "Loading..."
  • tests leaking into one another — i.e. tests that pass when run on their own but not together (maybe something to do with all the singletons used for rendering the app, like Start and RestService and/or with the Redux/zustand stores or local/sessionStorage not being cleared properly :shrug:)

In this MR, I bring some of the changes from !450 (closed) and fix various issues (notably with typing) that popped up at various times along my quest. I also upgrade two libraries mentioned in #103 that are now compatible with React 18.

We should have a chat about where to go next. We need to get past this one way or another so we can move forward in H5Web, but also for the long-term health of the Daiquiri UI project.

Edited by Axel Bocciarelli

Merge request reports

Pipeline #205382 passed

Pipeline passed for df6fc4f5 on towards-react-18

Merged by Stuart FisherStuart Fisher 2 months ago (Nov 5, 2024 10:22am UTC)

Loading

Pipeline #207206 passed

Pipeline passed for 8e395705 on main

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Axel Bocciarelli
  • Axel Bocciarelli
  • 92 90 }
    93 91 }
    94 92
    95 let Wrap: ReactNode = Fragment;
    • I've seen this pattern in a few places in the codebase. React 18 complained about this one, but generally-speaking, it is cleaner to either:

      • have two return statements and just duplicate the JSX that needs to be conditionnaly (like I did in my refactor); or
      • use composition by extracting the wrapping logic into a dedicated component and pass the JSX hat needs to be conditionally wrapped as children.
    • Stuart Fisher changed this line in version 3 of the diff

      changed this line in version 3 of the diff

    • Please register or sign in to reply
  • 1 1 import type { ColorMap, CustomDomain, Domain } from '@h5web/lib';
    2 2 import { ScaleType } from '@h5web/lib';
    3 import create from 'zustand';
  • 18 18 Start.start(store, layout);
    19 19 }
    20 20
    21 const view = render(<App />, { store });
    21 const view = await render(<App />, { store });
    22 22
    23 await waitFor(() =>
    24 expect(session.selector('auth', store.getState())).toEqual(true)
    25 );
    23 await waitFor(() => {
    24 expect(screen.queryByText('Starting Up')).not.toBeInTheDocument();
  • 4 4 // blobUrl
    5 5 window.URL.createObjectURL = jest.fn(() => '');
    6 6
    7 // popper.js
    8 jest.mock('popper.js', () => {
  • I've done my best to check for regressions, especially with Typeahead, but please do have a look as well.

  • Axel Bocciarelli requested review from @sfisher

    requested review from @sfisher

  • Axel Bocciarelli mentioned in merge request !450 (closed)

    mentioned in merge request !450 (closed)

  • Axel Bocciarelli mentioned in merge request !640 (closed)

    mentioned in merge request !640 (closed)

  • Stuart Fisher added 64 commits

    added 64 commits

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading