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

Detect the React and Flow versions relative to the file being linted. #2943

Merged
merged 1 commit into from Mar 10, 2021

Conversation

jcrosetto
Copy link
Contributor

This fixes #2218. This changes version.js to detect the React and Flow versions relative to the file being linted.

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! Can you perhaps add some tests without altering the existing ones?

@jcrosetto
Copy link
Contributor Author

You're welcome! I can't wait to get this in. :)

Basically all I did was swap out using process.cwd() with context.getFilename() when determining the version number. The existing tests needed to be updated because they relied on process.cwd() being used. I changed them to work with context.getFilename() instead.

I didn't add any new tests since the changes I made don't really change the expected output. There wasn't anything new or different to test per say. If you can think of something that needs testing I would be happy to add it.

@ljharb
Copy link
Member

ljharb commented Mar 9, 2021

The issue this fixes is that with nested dirs and deps, the wrong version is detected. To test this, I think we'd need some fixture files that replicate the issue, and verify that the version is detected properly.

@jcrosetto
Copy link
Contributor Author

That makes sense. I'll put together a test that replicates it.

@jcrosetto
Copy link
Contributor Author

jcrosetto commented Mar 9, 2021

Added tests that verify versions are correctly detected for both sibling and child projects. @ljharb Let me know if this adequately covers what you were thinking.

@ljharb
Copy link
Member

ljharb commented Mar 10, 2021

I'm still a bit confused; the existing tests use process.chdir, but presumably your fix would make them work even without that - so why can't the chdir be preserved?

@jcrosetto
Copy link
Contributor Author

jcrosetto commented Mar 10, 2021

It isn't needed anymore. The purpose of process.chdir was so that process.cwd would pick up the correct directory in version.js. You are correct that leaving it in there wouldn't affect the tests, but I think leaving it in would cause confusion as to what purpose it serves.

@ljharb ljharb force-pushed the 2218-react-version-monorepo branch 2 times, most recently from 257fa48 to eeb9273 Compare March 10, 2021 18:39
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.

I tweaked the test a bit to use sinon instead of reassignment, otherwise LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Warning: React version not specified when used with monorepo
2 participants