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

Bump remote-ui versions #1296

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Bump remote-ui versions #1296

merged 3 commits into from
Sep 21, 2023

Conversation

MitchLillie
Copy link
Contributor

@MitchLillie MitchLillie commented Aug 29, 2023

Background

This PR updates to @remote-ui/core@2.2 and @remote-ui/react@5. It also removes support for react@17 as these remote-ui updates were the only thing holding it back.

Due to the @remote-ui/react@5 dependency, consumers will now be required to manage their own versions of react and react-reconciler.

This should be marked as a breaking change. As noted below, I wasn't sure how to mark it as such using yarn changeset given that we are pointing to the unstable branch, as it prompts to release the first major version and I'm not sure that's right.

Relates to https://github.com/Shopify/app-ui/issues/228

Related PR: https://github.com/Shopify/web/pull/102560

Solution

(Describe your solution, why this approach was chosen, and what the alternatives/impacts may be)

🎩

  1. Create an app using the CLI
  2. yarn build-consumer <app directory here>
  3. yarn build-consumer-spin web (may need to build to a different location if web is not the right consumer of this project)
  4. Assert that extensions work properly.

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@MitchLillie
Copy link
Contributor Author

MitchLillie commented Aug 29, 2023

Notes:

I added a commit footer with "BREAKING CHANGE:" to denote this as a breaking change.

@MitchLillie MitchLillie force-pushed the ml/remote-ui branch 5 times, most recently from 30da967 to 98f84f4 Compare September 1, 2023 16:39
@MitchLillie MitchLillie marked this pull request as ready for review September 1, 2023 16:50
@github-actions

This comment has been minimized.

@MitchLillie MitchLillie requested review from a team, heltisace, NathanJolly, vctrchu, js-goupil, arigny, renerbaffa and lsit and removed request for a team September 1, 2023 16:53
@lsit lsit requested review from brianshen1990 and removed request for lsit September 1, 2023 16:58
Copy link
Contributor

@brianshen1990 brianshen1990 left a comment

Choose a reason for hiding this comment

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

From customer account web team, I am good with the upgrade:

  1. we upgraded remote-ui/core to 2.2.3 last week
  2. for "@remote-ui/react": we are using 4.5.x, we will see whether we need to upgrade if this cause any issue in the future https://github.com/Shopify/customer-account-web/blob/6510ab8c0f70747f377bb0b0b9dcb8de80415a85/app/package.json#L10
  3. for react, we are already using ^18.2 https://github.com/Shopify/customer-account-web/blob/main/package.json#L57

And also we are trying to switching from main to unstable, so it will not be a break change for us.

BREAKING CHANGE: Consumers of this package will be required to install `react-reconciler` to set their desired version of React.

See the `@remote-ui/react` release notes here for more information: https://github.com/Shopify/remote-ui/blob/main/packages/react/CHANGELOG.md#500
Copy link
Contributor

@jamesvidler jamesvidler left a comment

Choose a reason for hiding this comment

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

Do we need to change anything in the package.json of extensions that use this dependancy?

@@ -1,5 +1,5 @@
import React from 'react';
import {createMount} from '@quilted/react-testing';
import {createRender} from '@quilted/react-testing';
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to this file should be made within the private version of the package for checkout first before being moved into the public package.

@@ -53,7 +53,7 @@
"sideEffects": false,
"dependencies": {
"@remote-ui/async-subscription": "^2.1.12",
"@remote-ui/core": "2.1.x"
"@remote-ui/core": "^2.2.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this change already done in the private checkout package . 👍

@MitchLillie
Copy link
Contributor Author

@jamesvidler

Do we need to change anything in the package.json of extensions that use this dependancy?

Extensions shouldn't need to be updated, but any consumers (checkout-web?) may need to install react-reconciler. Did you have any luck tophatting this branch?

@NathanJolly NathanJolly removed their request for review September 14, 2023 13:35
Copy link
Contributor

@jamesvidler jamesvidler left a comment

Choose a reason for hiding this comment

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

I followed the tophatting steps using build-consumer <app> with a newly generated extension (using React 17 and then again with 18) without making ANY changes to checkout-web and the extension worked correctly.

This should unblock this PR. Though, we do still need to figure out how to have this co-exist with the private checkout-web package since that relies on React 17.

@MitchLillie MitchLillie merged commit a9401cc into unstable Sep 21, 2023
5 checks passed
@MitchLillie MitchLillie deleted the ml/remote-ui branch September 21, 2023 15:59
@jamesvidler
Copy link
Contributor

jamesvidler commented Sep 22, 2023

@MitchLillie
I just tested this in production using the unstable version with a newly generated app/extension (using yarn dev) without making any changes to the package.json and got this error:

TypeError: Cannot read properties of undefined (reading 'isBatchingLegacy')
...
ExtensionRenderError: Extension gid://shopify/LocalActivatedExtension/00 threw an error during rendering
...
react-reconciler.development.js:17731 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'isBatchingLegacy')

Using this package.json:

{
  "name": "checkout-ui",
  "private": true,
  "version": "1.0.0",
  "license": "UNLICENSED",
  "dependencies": {
    "react": "^17.0.0",
    "@shopify/ui-extensions": "unstable",
    "@shopify/ui-extensions-react": "unstable"
  },
  "devDependencies": {
    "@types/react": "^17.0.0"
  }
}

Not sure how I missed this when I was tophatting in spin 🤔.

If I update the dependancy to React 18, the error goes away:

{
  "name": "checkout-ui",
  "private": true,
  "version": "1.0.0",
  "license": "UNLICENSED",
  "dependencies": {
    "react": "^18.0.0",
    "@shopify/ui-extensions": "unstable",
    "@shopify/ui-extensions-react": "unstable"
  },
  "devDependencies": {
    "@types/react": "^18.0.0"
  }
}

Is this expected, that extensions will need to use React 18 going forward?

@jamesvidler
Copy link
Contributor

I tried using react-reconciler^0.27.0 for React 17 and that did not work either.

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.

4 participants