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

Jump to bottom

[New] Support "detect" option for React version setting #1978

Merged
merged 1 commit into from Oct 16, 2018
Merged

[New] Support "detect" option for React version setting #1978

merged 1 commit into from Oct 16, 2018

Conversation

alexzherdev
Copy link
Contributor

I think this resolves #1955.

alexzherdev
alexzherdev commented Sep 11, 2018
View reviewed changes
lib/util/version.js Outdated
const reactPath = resolve.sync('react', {basedir: process.cwd()});
const react = require(reactPath);
settingsVer = react.version;
} catch (e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what (if anything) needs to happen here, basically made a best guess.

ljharb
ljharb reviewed Sep 11, 2018
View reviewed changes
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.

This LGTM but is there any way to test for this?

lib/util/version.js Outdated Show resolved Hide resolved
lib/util/version.js Outdated
let settingsVer = context.settings.react.version;
if (settingsVer === 'detect') {
try {
const reactPath = resolve.sync('react', {basedir: process.cwd()});
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 this basedir is the default? probably best to specify it either way tho

alexzherdev
alexzherdev commented Sep 12, 2018
View reviewed changes
lib/util/version.js Outdated
try {
const reactPath = resolve.sync('react', {basedir: process.cwd()});
const react = require(reactPath);
settingsVer = react.version;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb does it make sense to cache this value for subsequent access?

Copy link
Member

Choose a reason for hiding this comment

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

yes, absolutely - good call.

@alexzherdev
Copy link
Contributor Author

This LGTM but is there any way to test for this?

I was gonna ask 😄
I could maybe imagine a unit-level test for getReactVersionFromContext. That would have to mock resolve.sync and require results - if this seems reasonable, any particular suggestions for libs to use for that?

@ljharb
Copy link
Member

ljharb commented Sep 12, 2018

hm, i don't think there is a good way :-/ however, you could construct a test directory with a package.json and a node_modules and a react dir with its own package.json, and require that?

@alexzherdev
Copy link
Contributor Author

Ah, I think I saw babel do something like that 👍

@ljharb
Copy link
Member

ljharb commented Sep 12, 2018

The import plugin does it in its tests, as well.

@alexzherdev
Copy link
Contributor Author

I added a couple of tests.
I tried caching the version into a local variable in the module and it worked, but then I didn't know how to reset that between different tests 🤔

EvHaus
EvHaus approved these changes Sep 13, 2018
View reviewed changes
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM

.gitignore Outdated Show resolved Hide resolved
@sebastian-nowak

This comment has been minimized.

Sign in to view
@ljharb ljharb merged commit 24044e0 into jsx-eslint:master Oct 16, 2018
@alexzherdev alexzherdev deleted the 1955-react-version-detect branch October 16, 2018 17:28
@dferber90 dferber90 mentioned this pull request Oct 17, 2018
Merged
@michaeltaranto michaeltaranto mentioned this pull request Nov 5, 2018
Closed
@tgdev
Copy link

tgdev commented Nov 5, 2018

This looks great. Any idea on when this might be published to NPM?

@alexzherdev
Copy link
Contributor Author

I'd love to see it published too 😄 but see #2026 (comment)

There’s no policy and no set timeline; things get released when maintainers have time to do so.

@lennymlzr lennymlzr mentioned this pull request Nov 23, 2018
Open
@ljharb
Copy link
Member

ljharb commented Dec 28, 2018

v7.12.0 is released.

@renovate renovate bot mentioned this pull request Dec 28, 2018
Merged
1 task
@renovate-bot renovate-bot mentioned this pull request Dec 28, 2018
Closed
1 task
This was referenced Jan 5, 2019
Closed
Merged
This was referenced Jan 6, 2019
Merged
Closed
Merged
Closed
Merged
Merged
Closed
Closed
Merged
Closed
Merged
Closed
Merged
Merged
Merged
@ghost ghost mentioned this pull request Jan 12, 2019
Merged
1 task
This was referenced Jan 13, 2019
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 17, 2019
Open
Open
This was referenced Sep 22, 2019
Open
Open
@snyk-bot snyk-bot mentioned this pull request Sep 30, 2019
Open
@alexzherdev alexzherdev mentioned this pull request Jun 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@EvHaus EvHaus EvHaus approved these changes

@yannickcr yannickcr Awaiting requested review from yannickcr

@lencioni lencioni Awaiting requested review from lencioni

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warning: React version not specified in eslint-plugin-react settings.
5 participants
@alexzherdev @ljharb @sebastian-nowak @tgdev @EvHaus