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

Ensure devtools e2e test use a compatible react-is version #22790

Merged
merged 2 commits into from
Nov 19, 2021

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Nov 19, 2021

Requires #22792
Fixes #22754 (comment)

The odd part was that the pretty-format used by Jest (node_modules/pretty-format) does install its own react-is from npm (node_modules/pretty-format/node_modules/react-is). But the pretty-format from playwright does not and therefore uses the top-level react-is (node_modules/react-is). However, the top-level react-is is the one checked-in i.e. the uncompiled version.

I used resolutions to enforce that pretty-format uses react-is from npm.

Note that only one test is passing at the moment. But at least they are runnable at the moment.

Summary

How did you test this change?

$ yarn build-for-devtools
# in packages/react-devtools-shell
$ yarn start
# in packages/react-devtools-inline
$ yarn start
$ yarn test:e2e

Copy link
Contributor

@bvaughn bvaughn 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 the fix!

@@ -146,5 +146,8 @@
"download-build": "node ./scripts/release/download-experimental-build.js",
"download-build-for-head": "node ./scripts/release/download-experimental-build.js --commit=$(git rev-parse HEAD)",
"download-build-in-codesandbox-ci": "cd scripts/release && yarn install && cd ../../ && yarn download-build-for-head || yarn build-combined --type=node react/index react-dom scheduler"
},
"resolutions": {
"**/@playwright/test/**/react-is": "^16.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this resolution be declared in the react-devtools-inline package (since that's the only thing that uses @playwright)? Just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I always declare them in the root. I tested it quickly and it didn't have any effect when declared in a nested package.

Though it may be preferred to declare it at the root in case another package uses @playwright/test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's fair.

@sizebot
Copy link

sizebot commented Nov 19, 2021

Comparing: bddbfb8...3015bf4

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 129.98 kB 129.98 kB = 41.56 kB 41.56 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.74 kB 134.74 kB = 42.96 kB 42.96 kB
facebook-www/ReactDOM-prod.classic.js = 424.49 kB 424.49 kB = 78.24 kB 78.24 kB
facebook-www/ReactDOM-prod.modern.js = 413.04 kB 413.04 kB = 76.50 kB 76.50 kB
facebook-www/ReactDOMForked-prod.classic.js = 424.49 kB 424.49 kB = 78.24 kB 78.24 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 3015bf4

@bvaughn bvaughn merged commit 149b420 into facebook:main Nov 19, 2021
@bvaughn bvaughn deleted the test/devtools-e2e branch November 19, 2021 15:18
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
…22790)

* Update lockfile by running `yarn install`

* Ensure devtools e2e test use a compatible react-is version
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.

4 participants