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

fix: add Profiler support for adapter-react-16 #2233

Merged
merged 5 commits into from Sep 8, 2019

Conversation

@andrewholsted
Copy link

commented Sep 7, 2019

Current Issue
When I follow the CONTRIBUTING.md docs on master and install React 16 (16.9.0), the tests fail due to errors with Profiler and ConcurrentMode. My understanding is that in 16.9 Profile no longer needs/has the _unstable prefix and _unstableConcurrentMode was removed as noted here.

Proposed Fix
This issue makes the Profiler change backwards compatible with from 16.6 through 16.9 and drops the test support for ConcurrentMode at 16.9.

Final Note
I haven't been able to actually test in my project that depends on this due to npm/yarn linking issues. I was still game to open the PR because the tests should pass on master now. I've only tested on 16 because it said CI would test all the other versions.

ljharb and others added 2 commits Aug 9, 2019
[enzyme-adapter-react-16] [new] add Profiler support for react v16.9+
 - ConcurrentMode was removed, and createRoot added, in v16.9+ (facebook/react#15532)
ljharb added 3 commits Sep 7, 2019

@ljharb ljharb force-pushed the andrewholsted:profiler-support branch from 49fccc8 to 370f614 Sep 7, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

rebased; fixed the last broken tests; and added some dep updates.

@ljharb
ljharb approved these changes Sep 8, 2019

@ljharb ljharb merged commit 370f614 into airbnb:master Sep 8, 2019

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 91.667%
Details
@chrisadubois

This comment has been minimized.

Copy link

commented Sep 9, 2019

@ljharb -- is there a time we could expect this PR to be in a release?

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

Not at the moment; I’m unable to do any releases for now while some permissions issues on the repo are worked out.

@chrisadubois

This comment has been minimized.

Copy link

commented Sep 9, 2019

@ljharb we'd like to test the functionality prior to the release with our project, can you offer a suggestion for how to link to the "enzyme-adapter-react-16" ? can pull the github remote url for master for the full enzyme project, but unable to for this separate package.

@andrewholsted

This comment has been minimized.

Copy link
Author

commented Sep 9, 2019

To tack on to what @chrisadubois said, our team is looking to give back to the open source community where we can so if there is anything we can help with on the permissioning issues let us know! We'll post back here if we figure out how to correctly npm link from the mono repo for the 16 adapter in case it helps anyone else.

@andrewholsted

This comment has been minimized.

Copy link
Author

commented Sep 16, 2019

bump for @ljharb. This is blocking one of our feature PRs in a project. Trying to give timelines to PMs but it's difficult without knowing when a next build will be published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.