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

@jcrosetto jcrosetto commented Mar 9, 2021

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

Copy link
Collaborator

@ljharb ljharb left a comment

Thanks! Can you perhaps add some tests without altering the existing ones?

@jcrosetto
Copy link
Contributor Author

@jcrosetto jcrosetto commented Mar 9, 2021

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
Collaborator

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

@jcrosetto jcrosetto commented Mar 9, 2021

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

@jcrosetto
Copy link
Contributor Author

@jcrosetto 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
Collaborator

@ljharb 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 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 jcrosetto:2218-react-version-monorepo branch 2 times, most recently from 257fa48 to eeb9273 Mar 10, 2021
@ljharb
ljharb approved these changes Mar 10, 2021
Copy link
Collaborator

@ljharb ljharb left a comment

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

@ljharb ljharb merged commit eeb9273 into yannickcr:master Mar 10, 2021
43 checks passed
43 checks passed
@github-actions
Automatic Rebase
Details
@github-actions
Require “Allow Edits”
Details
@github-actions
matrix
Details
@github-actions
pretest
Details
@github-actions
readme
Details
@github-actions
latest majors (15, 7)
Details
@github-actions
posttest
Details
@github-actions
latest majors (15, 6)
Details
@github-actions
latest majors (15, 5)
Details
@github-actions
latest majors (15, 4)
Details
@github-actions
latest majors (14, 7)
Details
@github-actions
latest majors (14, 6)
Details
@github-actions
latest majors (14, 5)
Details
@github-actions
latest majors (14, 4)
Details
@github-actions
latest majors (13, 7)
Details
@github-actions
latest majors (13, 6)
Details
@github-actions
latest majors (13, 5)
Details
@github-actions
latest majors (13, 4)
Details
@github-actions
latest majors (12, 7)
Details
@github-actions
latest majors (12, 6)
Details
@github-actions
latest majors (12, 5)
Details
@github-actions
latest majors (12, 4)
Details
@github-actions
latest majors (11, 7)
Details
@github-actions
latest majors (11, 6)
Details
@github-actions
latest majors (11, 5)
Details
@github-actions
latest majors (11, 4)
Details
@github-actions
latest majors (10, 7)
Details
@github-actions
latest majors (10, 6)
Details
@github-actions
latest majors (10, 5)
Details
@github-actions
latest majors (10, 4)
Details
@github-actions
latest majors (9, 6)
Details
@github-actions
latest majors (9, 5)
Details
@github-actions
latest majors (9, 4)
Details
@github-actions
latest majors (8, 6)
Details
@github-actions
latest majors (8, 5)
Details
@github-actions
latest majors (8, 4)
Details
@github-actions
latest majors (7, 5)
Details
@github-actions
latest majors (7, 4)
Details
@github-actions
latest majors (6, 5)
Details
@github-actions
latest majors (6, 4)
Details
@github-actions
latest majors (5, 4)
Details
@github-actions
latest majors (4, 4)
Details
@github-actions
node 4+
Details
@jcrosetto jcrosetto deleted the jcrosetto:2218-react-version-monorepo branch Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants