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

[core] fix(ResizeSensor): compatibility with React 18 #5362

Merged

Conversation

PurkkaKoodari
Copy link
Contributor

@PurkkaKoodari PurkkaKoodari commented Jun 7, 2022

Fixes #5313 & another bug

Checklist

  • Includes tests: This is not testable easily without testing against React 18, which would be a pretty big change given that the build is currently hard-coded against React 16.
  • Update documentation: I didn't touch the docs, as this doesn't change the user-facing API at all.

(If you think this needs tests or docs, I'll update the PR.)

Changes proposed in this pull request:

Firstly, this fixes #5313 by defining an explicit return type for render().

The difference between React 18 and 16 here seems to be that React 18 specifies ReactFragment = Iterable<ReactNode>, while React 16 specifies {} | ReactNodeArray. The definition of only drops the array, so the compiled type declaration is an union of what's left of ReactNode. That breaks when the newer @types/react doesn't allow {}. Specifying ReactNode should allow the union at compile time, and then the compiled render(): ReactNode is compatible with any version of React.

A quick grep for React.Children.only suggests that packages/table/src/common/loadableContent.tsx and packages/table/src/interactions/draggable.tsx are also affected, but I didn't have time to test them right now.

Secondly, this makes ResizeSensor work with React 18 StrictMode and its "reusable state", which essentially runs effects twice - against their own docs for componentWillUnmount. (The docs make this very enjoyable to debug.)

The current code assumes that the component is dumped after componentWillUnmount so it doesn't bother to clear this.element. This permanently disables the observer since observeElement will then think the element is already being observed. I added this.element = null to make it "reusable".

Reviewers should focus on:

If this breaks something subtle I didn't notice, I guess?

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @PurkkaKoodari! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Sweet, this looks like a clean solution to the problem, thanks @PurkkaKoodari!

@adidahiya adidahiya merged commit a32d66c into palantir:develop Jun 8, 2022
@adidahiya adidahiya changed the title fix: make ResizeSensor compatible with React 18 [core] fix(ResizeSensor): compatibility with React 18 Jun 8, 2022
@PetrusAsikainen
Copy link
Contributor

PetrusAsikainen commented May 24, 2023

@adidahiya Seems this change never made it to v5?

@adidahiya
Copy link
Contributor

@PetrusAsikainen good catch! Fixing that here: #6191

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

Successfully merging this pull request may close these issues.

[React 18] ResizeSensor cannot be used as a JSX component
4 participants