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 the right versions of React / adapters are installed #2116

Merged
merged 1 commit into from May 7, 2019

Conversation

Projects
None yet
2 participants
@ahuth
Copy link
Contributor

commented May 6, 2019

There's a weird issue where partial React versions can result in the wrong versions of React being installed. This happens because semver.intersects is returning something weird when both are true:

  • A partial version of React is used - for example, 16.3
  • And it's compared against a version with prerelease specifiers in it

See this truth table which highlights the issue.

In this PR we:

  • Separate out a function for determining the right adapter to install (which makes testing it easier)
  • Convert to using semver.satisfies from semver.intersects

@ljharb , what do you think?

import semver from 'semver';

export default function getAdapterForReactVersion(reactVersion) {
const normalizedVersion = semver.coerce(reactVersion);

This comment has been minimized.

Copy link
@ahuth

ahuth May 6, 2019

Author Contributor

Need this because something like "16.3" isn't a valid version, per semver.

Their docs state that coerce is guaranteed to generate a version as long as there is a number in the string.

@@ -0,0 +1,86 @@
import { expect } from 'chai';
import getAdapterForReactVersion from '../../enzyme-adapter-react-helper/src/getAdapterForReactVersion';

This comment has been minimized.

Copy link
@ahuth

ahuth May 6, 2019

Author Contributor

Feel like I should've been able to do

import getAdapterForReactVersion from 'enzyme-adapter-react-helper/src/getAdapterForReactVersion';

Didn't work, though 🤔

This comment has been minimized.

Copy link
@ljharb

ljharb May 6, 2019

Member

i think if you add enzyme-adapter-react-helper as a dev dep of enzyme-test-suite, that this will work?

This comment has been minimized.

Copy link
@ahuth

ahuth May 7, 2019

Author Contributor

Oh, that would make sense

@ljharb
Copy link
Member

left a comment

Thanks, this looks great.

return 'enzyme-adapter-react-13';
}

return 'enzyme-adapter-react-16';

This comment has been minimized.

Copy link
@ljharb

ljharb May 6, 2019

Member

we didn't used to have this fallback here - should it throw instead?

This comment has been minimized.

Copy link
@ahuth

ahuth May 7, 2019

Author Contributor

Yeah, that's probably better than randomly getting the -16 adapter

@ljharb

ljharb approved these changes May 7, 2019

Copy link
Member

left a comment

Thanks!

@ljharb ljharb merged commit cdd304d into airbnb:master May 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ljharb added a commit that referenced this pull request May 8, 2019

[enzyme-adapter-react-helper] v1.3.4
 - [fix] `install`: Ensure the right versions of React / adapters are installed (#2116)
 - [dev deps] update `eslint`, `semver`, `rimraf`, `eslint-plugin-react`, `eslint-plugin-import`
 - [build] include source maps

ahuth added a commit to ahuth/enzyme that referenced this pull request May 15, 2019

[enzyme-adapter-react-helper] Ensure that the correct version of an a…
…dapter gets installed

Particularly when only specifying a major version of React to match against. When we pass in something like `16`, we really mean "any React 16 version". Essentially >= 16 and < 17.

To get that behavior, we need to treat our input as a Range, and get its intersection with the value we test against. This is basically where we were prior to airbnb#2116, except that pre-release version support has been dropped, which fixes the issue that PR was attempting to fix.

ahuth added a commit to ahuth/enzyme that referenced this pull request May 15, 2019

[tests] Add failing tests for the behavior we want out of enzyme-adap…
…ter-react-helper

For when only a major version number is provided. This regressed in airbnb#2116 (while fixing another issue).

ahuth added a commit to ahuth/enzyme that referenced this pull request May 24, 2019

[tests] Add failing tests for the behavior we want out of enzyme-adap…
…ter-react-helper

For when only a major version number is provided. This regressed in airbnb#2116 (while fixing another issue).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.