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

Support React 18 #6743

Merged
merged 26 commits into from Oct 7, 2022
Merged

Support React 18 #6743

merged 26 commits into from Oct 7, 2022

Conversation

ryanwilsonperkin
Copy link
Member

@ryanwilsonperkin ryanwilsonperkin commented Jul 22, 2022

WHY are these changes introduced?

Fixes #5477

Adds explicit support for React 18 in the component library, website, and figma plugin. Fixes bugs and type errors that result.

WHAT is this pull request doing?

There are a few notable changes that we make in order to work with new behaviours in React 18:

  1. The AfterInitialMount helper component gets a new prop onMount callback that will be invoked after the initial mount. This lets us avoid a timing issues in IndexTable that happens based on new mounting & effect processing behaviour that would otherwise cause a callback to invoked before a ref was actually available in the DOM.
  2. An updated version of @shopify/react-testing as well as proper use of the act method throughout our tests. act was previously more lenient but gets stricter in React 18 about exactly how and when it should be invoked.
  3. Removed unnecessary use of resetModules in some of our tests which was interfering with a global behaviour of the @shopify/react-testing library
  4. Use a ref instead of state in the Konami code easter egg component. In React 18, state changes are now batched in more places, meaning that this handler was being invoked with stale references and was unable to track state changes appropriately. A ref works better in this scenario because we're trying to use a mutable store of value to ensure we're not relying on a stale state reference.
  5. Fix type checking issues, most notably that useCallback functions now require parameter signatures and functional components no longer implicitly receive a children prop, and need to declare it explicitly now.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2022

size-limit report 📦

Path Size
polaris-react-cjs 209.01 KB (+2.09% 🔺)
polaris-react-esm 135.51 KB (+3.29% 🔺)
polaris-react-esnext 190.87 KB (+2.31% 🔺)
polaris-react-css 41.52 KB (0%)

@ryanwilsonperkin ryanwilsonperkin force-pushed the react-18-support branch 2 times, most recently from 13f713a to 6dc27a8 Compare September 6, 2022 13:30
@ryanwilsonperkin ryanwilsonperkin marked this pull request as ready for review September 6, 2022 13:50
@kyledurand kyledurand self-requested a review September 12, 2022 15:43
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

This looks great @ryanwilsonperkin thank you for taking this on. I think next you'll want to /snapit for testing in web. Let me know if you want a hand with that

.changeset/kind-bears-pretend.md Outdated Show resolved Hide resolved
Comment on lines +252 to +253
} else {
actionMarkup = primaryAction;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

actionMarkup will be used below and must be a ReactNode, but the value that gets passed in from primaryAction can be either a ReactNode or an "interface" object with some properties on it. So what's happening here is that we're initially assigning it to T | ReactNode and then in the isInterface condition we're refining that to just ReactNode, however for some reason the type assertion of the isInterface utility isn't working correctly so lower down when we try to use this as a ReactNode TypeScript still believes that it's either T | ReactNode.

React itself used to be more permissive about this, but is stricter in v18 and flags this as a potential error. By telling the compiler that it can only ever be a ReactNode and then assigning it as such in the two different conditions, we make the compiler happy, and don't change the functionality of the code.

@BPScott
Copy link
Member

BPScott commented Oct 7, 2022

Rebased atop latest main. Only conflicts was some small ordering adjusts in package.json files

@chloerice chloerice added dependencies Pull requests that update a dependency file Key dependency and removed dependencies Pull requests that update a dependency file labels Oct 7, 2022
React 18 changes up the timing of when certain effects will fire, we
have cases where we use AfterInitialMount to mount something which
contains a ref, but that ref won't be available to the parent component
any longer during the same synchronous effect cycle.

This change adds a callback prop to AfterInitialMount so that we can
tell it to invoke the callback after it has rendered its children and we
can use that to invoke the appropriate behaviour instead of doing it in
a useEffect
Invoking resizeTableScrollBar prior to these refs being mounted is a
no-op, we need to tell AfterInitialMount to invoke the function once its
rendered the children so that we know the refs that we're operating on
will be available.
Fixes broken tests by ensuring that the changes that are caused by these
actions are updated and reflected in the component under test.
There's no need to reset modules here because the reference to NODE_ENV
in the system under test is within the component and is not at the
module level. It will be evaluated every time at runtime and can be
safely mocked out without needing to reset the modules.

This fix is needed because resetting the modules in this way results in
the components under test being unmounted.
In React 18, state updates are no longer flushed synchronously to the
component within event handlers. Rather than use state for this purpose,
we should use a mutable ref so that we can easily increment the value
and be sure that we're always referencing the latest value and not
looking at a stale "position" number that hasn't yet received the update
from the last event that was processed.
This test previously included the messaging that the setTimeoutSpy would
be invoked twice because of a specific behaviour of react scheduler.
This is no longer true under react 18 and we can use the regular
assertion of "toHaveBeenCalled" instead of specifying that it happens
twice.
Contains a fix to work properly with the version of @types/react that
we're now using
In React 18 types, useCallback wrapped functions require their
parameter types to be explicitly defined. Previously these would end up
being implicit any.

In one case here I've opted to remove the callback altogether because
all it did was wrap a setState with the same value, and that setState
function is already stable so it doesn't require the useCallback
wrapper.
ryanwilsonperkin and others added 8 commits October 7, 2022 13:57
Previously content was assigned to a type "A | B" and then went into a
conditional block that narrowed it to just "A". But the rendered content
beneath still believed the type to be "A | B" which was allowed
previously where React would treat it as potentially renderable, but it
is now stricter and requires us to do more explicit type narrowing. We
can do that by assigning the value conditionally in the two different
cases so that it knows that it will _always_ be narrowed to "A"
We need to let wrapWithComponent know that in addition to the props that
the component accepts, it might also receive the prop "key" prop which
comes from JSX intrinsic attributes.
By just using the `Parameter<typeof useCallback>[0]` previously this was
being evaluated as simply "Function", so the value that was returned
from useDeepCallback lost any of the particular typing of the function
that it wrapped.

By using this generic approach, the return value will receive the proper
type signature.
Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>
@chloerice
Copy link
Member

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2022

🫰✨ Thanks @chloerice! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221007181635
yarn add @shopify/polaris@0.0.0-snapshot-release-20221007181635

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this upgrade @ryanwilsonperkin!! This fixes some bugs affecting the admin, including the IndexTable scrollbar bug 🎉 (Spinstance)

@BPScott
Copy link
Member

BPScott commented Oct 7, 2022

@ryanwilsonperkin is vacationing (since Sept 27th, calendars suggest he'll be back on Tuesday). If the polaris crew is happy then I'm confident that this is good to merge without waiting for him to return and do the what-have-i-missed catchup.

@chloerice chloerice merged commit 8d440aa into main Oct 7, 2022
@chloerice chloerice deleted the react-18-support branch October 7, 2022 21:18
@github-actions github-actions bot mentioned this pull request Oct 7, 2022
chloerice pushed a commit that referenced this pull request Oct 7, 2022
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris@10.7.0

### Minor Changes

- [#6743](#6743)
[`8d440aa6b`](8d440aa)
Thanks [@ryanwilsonperkin](https://github.com/ryanwilsonperkin)! - Add
support for React 18


- [#7359](#7359)
[`6be4436d0`](6be4436)
Thanks [@clarkjennings](https://github.com/clarkjennings)! - - Updated
font size for square `Avatar` with 3-letter initials

## @shopify/plugin-polaris@0.0.9



## polaris-for-figma@0.0.23

### Patch Changes

- [#6743](#6743)
[`8d440aa6b`](8d440aa)
Thanks [@ryanwilsonperkin](https://github.com/ryanwilsonperkin)! - Add
support for React 18

- Updated dependencies
\[[`8d440aa6b`](8d440aa),
[`6be4436d0`](6be4436)]:
    -   @shopify/polaris@10.7.0

## polaris.shopify.com@0.21.1

### Patch Changes

- [#6743](#6743)
[`8d440aa6b`](8d440aa)
Thanks [@ryanwilsonperkin](https://github.com/ryanwilsonperkin)! - Add
support for React 18


- [#7370](#7370)
[`58cdc1671`](58cdc16)
Thanks [@chloerice](https://github.com/chloerice)! - Removed 'About
Polaris' page from the Foundations section, as the 'Getting started'
section covers this

- Updated dependencies
\[[`8d440aa6b`](8d440aa),
[`6be4436d0`](6be4436)]:
    -   @shopify/polaris@10.7.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Oct 7, 2022
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.

In polaris-react, bump react version in peerDependencies to 18.0.0
5 participants