Skip to content

Conversation

@kyledurand
Copy link
Member

WHY are these changes introduced?

Fixes #4271

WHAT is this pull request doing?

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

@kyledurand kyledurand self-assigned this Aug 25, 2021
@kyledurand kyledurand added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Aug 25, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2021

size-limit report

Path Size
cjs 163.35 KB (-0.07% 🔽)
esm 96.08 KB (-0.12% 🔽)
esnext 142.98 KB (-0.08% 🔽)
css 34.52 KB (0%)

@BPScott
Copy link
Member

BPScott commented Aug 25, 2021

Remember to bump the peerdeps too. We'll want to support both 16 and 17 at the same time because cordinating "all libraries used by an app must support react 17 before the app can go to 17" is hard and we want apps that are stuck on 16 while they wait for their other dependencies to update to not be locked out of this new polaris version, so the range is

"react": "^16.14.0 || ^17.0.0",
"react-dom": "^16.14.0 || ^17.0.0",

Base automatically changed from v7.0.0-release to main August 26, 2021 21:05
@paulgrieselhuber
Copy link

Is there any update on when this should be finalized?

@kyledurand
Copy link
Member Author

We're still hoping for this week. We're in the process of removing all references to test-utilities/legacy which means fully migrating our tests @shopify/react-testing. Then we can remove enzyme and support both react 16 and 17

@kyledurand kyledurand force-pushed the react-17 branch 2 times, most recently from 3403c6e to f386b09 Compare September 3, 2021 20:31
@kyledurand kyledurand marked this pull request as ready for review September 3, 2021 20:31
@kyledurand
Copy link
Member Author

kyledurand commented Sep 3, 2021

Small update: We just shipped the last PR to remove the legacy test helpers. I've updated this PR to remove enzyme and a few things we no longer need.

@BPScott
Copy link
Member

BPScott commented Sep 6, 2021

I think we should keep that environments config file. It is useful to suppress lots of long console errors about window.scroll being not implemented and the testID prop not being valid, and various internal deprecation messages.

Compare the test CI of 2574f11 being 394 lines long, vs test CI of 726a513 being 3308 lines long. The less noise we have in a passing test suite then the easier it will be to find and see problems in a failing test suite.

And tangentially:

  • Looks like mutationobserver was added to jsdom in v13 a few years back, so mutationobserver-shim can be removed from the environment.ts and from our package.json
  • Looks like we can remove a bunch of those mentions of testID - when they're all removed we can also remove this file that teaches TS that is an acceptable prop, but that doesn't need to be done in this PR.

@kyledurand kyledurand force-pushed the react-17 branch 2 times, most recently from 608b3af to 65f08b2 Compare September 7, 2021 14:59
@alex-page alex-page self-assigned this Sep 7, 2021
@paulgrieselhuber
Copy link

🎉 Many thanks for getting this in!

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Hecks yea, looking great!

There's a few console errors in the test output but I think they were there before too.

@alex-page alex-page merged commit 8d79d05 into main Sep 7, 2021
@alex-page alex-page deleted the react-17 branch September 7, 2021 17:08
@paulgrieselhuber
Copy link

Nice to see this merged. Any idea when it will be in a release published to npm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ship React 17

4 participants