React 16 + Ref Support #106
Conversation
| "react-dom": "^15.6.1" | ||
| "prop-types": "^15.6.2", | ||
| "react": "^16.5.2", | ||
| "react-dom": "^16.5.2" |
There was a problem hiding this comment.
A while back we discussed adding better integration test coverage of multiple React versions. Might be worth a separate PR that adds coverage for both React 15 and 16 and both create-react-app 1 and 2.
| "<rootDir>/packages/" | ||
| ], | ||
| "setupFiles": [ | ||
| "raf/polyfill" |
There was a problem hiding this comment.
React 16 requires a requestAnimationFrame polyfill for tests to run properly.
There was a problem hiding this comment.
Looks like Jest 21.3.x included the polyfill by default. Maybe we can bump the version of Jest here (or a separate PR?) jestjs/jest#4568 (comment)
There was a problem hiding this comment.
Sure thing, I’ll toss up a separate PR for that next week. I’m off on vacation till then :)
| "react": "^15.4.0", | ||
| "react-dom": "^15.4.0" | ||
| "react": "^15.4.0 || ^16.0.0", | ||
| "react-dom": "^15.4.0 || ^16.0.0" |
There was a problem hiding this comment.
This allows users to have either React 15 or React 16 in their projects without getting any peer dependency warnings.
| const assets = await compile(percyConfig); | ||
|
|
||
| const environment = new Environment(); | ||
| const runtimeEntry = findEntryPath(assets, EntryNames.runtime); |
There was a problem hiding this comment.
The runtime entry contains a manifest webpack uses to make sure imports work correctly across entries now that we're splitting some dependencies out into the vendor bundle. The vendor bundle contains react and react-dom, so that they're not included in multiple scripts.
| /* eslint-enable react/display-name, react/prop-types */ | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.error = jest.fn(); |
There was a problem hiding this comment.
This test purposefully causes an error to get thrown so we can verify it's getting rendered. React 16 has a new mechanism for catching errors and prints a console error here, but it's not really useful in the context of this test.
|
@timhaines any ideas why the |
Robdel12
left a comment
Robdel12
left a comment
There was a problem hiding this comment.
Other than the raf polyfill everything looks good. I wonder why the snapshots are blank though 🤔
| "<rootDir>/packages/" | ||
| ], | ||
| "setupFiles": [ | ||
| "raf/polyfill" |
There was a problem hiding this comment.
Looks like Jest 21.3.x included the polyfill by default. Maybe we can bump the version of Jest here (or a separate PR?) jestjs/jest#4568 (comment)
Robdel12
left a comment
Robdel12
left a comment
There was a problem hiding this comment.
I ran the changes locally and compared snapshots between this & the released version. Everything looks good to me! 👍
I've been spending a bit of time this week seeing if we can get SeatGeek working with Percy for a project I'm working on. I'm pretty close, but was running into a few issues which I've addressed in this PR:
@percy/reactwas restricting peer dependencies to only React v15, but React v16 has been out for a year. I've updated the peer dependencies allow both v15 and v16, and updated the unit tests here to run against v16.I discovered that
refs don't work properly because we were loading two copies of React on the page when snapshotting (it was included in both thesnapshotsandrenderbundles). I've updated the webpack config to extractreactandreact-dominto avendorbundle to prevent it from loading multiple times, and added some integration tests here to verify that refs work. The tests failed before my changes, and now render properly with my updates.