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

Revert "chore(deps-dev): bump @types/react from 18.0.21 to 18.0.25 (#… #1831

Conversation

robinmetral
Copy link
Contributor

This reverts commit 0650528.

Purpose

This version bump caused compatibility issues with web apps using Circuit on React 17.

It looks like the underlying HTML element types for components (such as HTMLAttributes<HTMLParagraphElement> for Body) is incompatible between React 17 and 18 types:

Screenshot 2022-11-08 at 14 05 32

The error messages point to two recent changes in the React 18 types:

Approach and changes

Before downgrading I tried:

  • to upgrade the React types to v18 in the web app, but this caused too many other clashes with the internal React 17 version (and upgrading React there is not an option yet)
  • to redeclare/override the Circuit UI types, without success

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@vercel
Copy link

vercel bot commented Nov 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
oss-circuit-ui ✅ Ready (Inspect) Visit Preview Nov 8, 2022 at 2:16PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Nov 8, 2022

⚠️ No Changeset found

Latest commit: 3f38b9b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/sumup-oss/circuit-ui/new/revert-1825-dependabot/npm_and_yarn/types/react-18.0.25?filename=.changeset/soft-mangos-work.md&value=---%0A%22%40sumup%2Fcircuit-ui%22%3A%20patch%0A---%0A%0ARevert%20%22chore(deps-dev)%3A%20bump%20%40types%2Freact%20from%2018.0.21%20to%2018.0.25%20(%23%E2%80%A6%0A)

@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #1831 (3f38b9b) into main (2a8c6e6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1831   +/-   ##
=======================================
  Coverage   91.20%   91.20%           
=======================================
  Files         170      170           
  Lines        3662     3662           
  Branches     1220     1220           
=======================================
  Hits         3340     3340           
  Misses        303      303           
  Partials       19       19           

@robinmetral
Copy link
Contributor Author

Update: I noticed that the nonce change (DefinitelyTyped/DefinitelyTyped#62879) was released in @types/react@17.0.52 and upgrading the package in the web app solves the issue.

This wasn't done for the onResize change (DefinitelyTyped/DefinitelyTyped#63076), see DefinitelyTyped/DefinitelyTyped#63076 (comment). If we can't contribute this to the @types package we can surely patch it in the web app, so this revert is probably not necessary—will keep the PR open in draft status for visibility

@sumup-oss sumup-oss deleted a comment from sumup-clark bot Nov 8, 2022
@robinmetral
Copy link
Contributor Author

robinmetral commented Nov 8, 2022

A short-term fix for the issue is to extend the namespace of the React 17 types in apps using Circuit UI:

import { ReactEventHandler } from 'react';

declare global {
  namespace React {
    interface DOMAttributes<T> {
      onResize?: ReactEventHandler<T> | undefined;
      onResizeCapture?: ReactEventHandler<T> | undefined;
    }
  }
}

Downsides here:

  • Extending namespaces feels a bit hacky
  • It makes it seem like onResize is available in React 17, while it isn't. (This isn't great but unlikely to cause issues, since onResize isn't commonly used. Even if it were, a developer would quickly notice that their onResize handlers are ignored in React 17.)

Let's see if anything better comes out of the conversation over at DefinitelyTyped. But even if nothing does, it feels like downgrading types isn't the solution here, hence, closing this PR.

@robinmetral robinmetral closed this Nov 8, 2022
@robinmetral robinmetral deleted the revert-1825-dependabot/npm_and_yarn/types/react-18.0.25 branch November 8, 2022 16:24
robinmetral pushed a commit that referenced this pull request Dec 1, 2022
This solves TS issues with nonce. See #1831
robinmetral pushed a commit that referenced this pull request Dec 1, 2022
* Update sumup packages

* Upgrade React and React types

This solves TS issues with nonce. See #1831

* Remove noMargin from template

* Fix docs ref
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant