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
Merged

Ensure the right versions of React / adapters are installed #2116

merged 1 commit into from
May 7, 2019

Conversation

ahuth
Copy link
Contributor

@ahuth ahuth 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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';
Copy link
Contributor Author

@ahuth ahuth May 6, 2019

Choose a reason for hiding this comment

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

Feel like I should've been able to do

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

Didn't work, though 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that would make sense

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great.

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

return 'enzyme-adapter-react-16';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@ljharb ljharb merged commit cdd304d into enzymejs:master May 7, 2019
ljharb added a commit that referenced this pull request May 8, 2019
 - [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
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.

2 participants