Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚧(frontend) migrate to react-query v4 & react 18 #1812

Merged
merged 4 commits into from Nov 15, 2022

Conversation

jbpenrath
Copy link
Member

@jbpenrath jbpenrath commented Oct 24, 2022

Purpose

Until now, we cannot upgrade to React 18 due to an incompatibility with react-query v3. React query v4 has been released and add the support of React 18 that's mean we are now able to migrate our frontend codebase to React 18 ! πŸŽ‰

Proposal

  • Migrate to react-query v4
    • Fix console.error for http expected errors.
  • Migrate to react 18
    • Replace ReactDOM.render
  • Upgrade testing-library/react to its latest version
  • Unpin all dependencies above from renovate configuration
  • Write tests for createTestQueryClient
  • Make sure Storybook is ok
  • Fix yarn warnings

❌ Failed tests

Never surrender.

26/10 53 failed, 303 passed,
28/10 πŸ”© 24 failed, 332 passed
31/10 ☠️ 18 failed, 338 passed

-> 03/10 All tests are fixed 😎

@jbpenrath jbpenrath added the dependencies Dependency-related tasks. label Oct 24, 2022
@jbpenrath jbpenrath changed the title 🚧(frontend) migrate to react-query v4 🚧(frontend) migrate to react-query v4 & react 18 Oct 24, 2022
@NathanVss NathanVss force-pushed the deps/migrate-to-react-18 branch 2 times, most recently from a786d0e to a0bc91c Compare November 4, 2022 10:56
@NathanVss
Copy link
Member

BUG:

  • Open a new tab
  • Open console
  • Copy paste localhost:8070/en/dashboard/preferences
  • The dashboard loads .. a suddently errors appear in the console and the browser is redirected to the website home.

Capture d’écran 2022-11-04 aΜ€ 14 58 51

@jbpenrath
Copy link
Member Author

jbpenrath commented Nov 7, 2022

BUG:
Components: CourseProductsList
Cannot be reproduce on master: βœ…

Steps to reproduce

  • Open Richie as an Anonymous User
  • Go to a course detail page displaying a Joanie product
  • Click on Log In button into the header or on Login to purchase button within the product widget
  • The widget disappear and an endless spinner is displayed

2022-11-07 14 48 53

Note

This bug has been reported before the upgrade by @NamFra, but I have never be able to reproduce it.
https://trello.com/c/8H2Fkdh9

@NathanVss NathanVss force-pushed the deps/migrate-to-react-18 branch 8 times, most recently from 75c0da8 to a8bdb89 Compare November 8, 2022 14:00
@NathanVss NathanVss marked this pull request as ready for review November 8, 2022 14:04
CHANGELOG.md Outdated Show resolved Hide resolved
docs/joanie-connection.md Outdated Show resolved Hide resolved
src/frontend/js/components/Dashboard/index.tsx Outdated Show resolved Hide resolved
src/frontend/js/components/Modal/index.tsx Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
import { FC, useMemo } from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a global remark. When you import a type try to prefix it with type. It can help to speed up build by ignoring all types import.

Suggested change
import { FC, useMemo } from 'react';
import { type PropsWithChildren, useMemo } from 'react';

Copy link
Member

Choose a reason for hiding this comment

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

As PropsWithChildren is already exported as a type isn't it unuseful ?

Copy link
Member

Choose a reason for hiding this comment

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

As I read in this article https://javascript.plainenglish.io/leveraging-type-only-imports-and-exports-with-typescript-3-8-5c1be8bd17fb it says: Note that you can also use β€œexport type” to indicate that some export will only ever be used as a type annotation/declaration.

But indeed if you have "official" documentation resources for this particular point I will be really interested πŸ™

Copy link
Member

Choose a reason for hiding this comment

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

Even https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export states This feature is something most users may never have to think about; however, if you’ve hit issues under [isolatedModules](https://www.typescriptlang.org/tsconfig#isolatedModules), TypeScript’s transpileModule API, or Babel, this feature might be relevant. so do we really need to put import type ?

src/frontend/js/data/useStaticFilters/index.spec.tsx Outdated Show resolved Hide resolved
update all required dependencies to support react 18.
react 18 forces to explicitly name the `children` attribute so let's update
components and hooks.
Copy link
Member

@NathanVss NathanVss left a comment

Choose a reason for hiding this comment

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

I approve myself ! βš–οΈ

most of the tests broke due to the changes induced by the new rendering
behavior of react 18 and react-query v4, so we changed the approach
that we had with tests to an approach more based on `waitFor` in order
to have tests the most render-agnostic as possible.
This bug was occuring when clicking on the button "Login to purchase"
causing a removal of existing react-query queries. This was caused by
premature removal of the pending useCourse query which was never notified
that it had been removed during its loading, resulting in an infinite
loading state.
@NathanVss NathanVss merged commit a47653b into master Nov 15, 2022
@NathanVss NathanVss deleted the deps/migrate-to-react-18 branch November 15, 2022 10:08
@jbpenrath jbpenrath mentioned this pull request Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Dependency-related tasks. needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants