Skip to content

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

Merged
ljharb merged 5 commits intoenzymejs:masterfrom
holsted:profiler-support
Sep 8, 2019
Merged

fix: add Profiler support for adapter-react-16#2233
ljharb merged 5 commits intoenzymejs:masterfrom
holsted:profiler-support

Conversation

@holsted
Copy link
Copy Markdown

@holsted holsted 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
Copy link
Copy Markdown
Member

ljharb commented Sep 7, 2019

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

@ljharb ljharb merged commit 370f614 into enzymejs:master Sep 8, 2019
@chrisadubois
Copy link
Copy Markdown

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

@ljharb
Copy link
Copy Markdown
Member

ljharb 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
Copy link
Copy Markdown

@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.

@holsted
Copy link
Copy Markdown
Author

holsted 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.

@holsted
Copy link
Copy Markdown
Author

holsted 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.

@holsted
Copy link
Copy Markdown
Author

holsted commented Sep 30, 2019

Last plea for a deploy of latest to NPM @ljharb. We're looking at forking or going with a different library as we really want to have the profiler support metrics in local dev and when our automation runs.

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Oct 2, 2019

@andrewholsted i'm sorry, i'm still unable to publish. You can install the adapter from any git sha, however, so there should be no need for either forking or another library.

@holsted
Copy link
Copy Markdown
Author

holsted commented Oct 2, 2019

@ljharb The sha won't have the "built" version though, correct? Or am I missing something? We tried installing from github (and linking locally) but ran into several babel type issues.

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Oct 2, 2019

@andrewholsted this is true, but you can add a postinstall script to cd into the package, and run npm install && npm run build. it's not ideal but it works in the meantime.

@holsted
Copy link
Copy Markdown
Author

holsted commented Oct 4, 2019

I'll take a look at that. Building locally and doing npm link was giving us the babel issues. I can try the above, instead.

@holsted
Copy link
Copy Markdown
Author

holsted commented Oct 7, 2019

@ljharb - no dice

Exit code: 127
Command: [ -n "${TRAVIS-}" ] || (npm run clean-local-npm && npm link npm && lerna bootstrap)
Arguments: 
Directory: <redacted> /node_modules/enzyme-adapter-react-16
Output:
npm WARN lifecycle The node binary used for scripts is /var/folders/3d/nx667hg51cq89cqvfz9ytmrw0000gn/T/yarn--1570467233245-0.6325175962059095/node but npm is using <redacted>.nvm/versions/node/v10.16.0/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.

We've tried everything we can think of. Please post back when you can publish a new version. I don't understand what could be holding up the process so badly though.

ljharb added a commit that referenced this pull request Oct 9, 2019
 - [new] add Profiler support for react v16.9+ (#2233)
 - [new] add `wrapInvoke` to adapter (#2158)
 - [refactor] use `enzyme-shallow-equal`
 - [dev deps] update `eslint`, `eslint-plugin-react`, `eslint-config-airbnb`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`, `eslint-plugin-react`, `safe-publish-latest`
 - [meta] Update airbnb.io URLs to use https (#2222)
@ljharb
Copy link
Copy Markdown
Member

ljharb commented Oct 9, 2019

@andrewholsted v1.15.0 of the react 16 adapter has been released.

@chrisadubois
Copy link
Copy Markdown

@ljharb that worked, thank you

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.

3 participants