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

Improve .babelrc lookup #93

Merged
merged 4 commits into from Oct 4, 2018
Merged

Conversation

TheCycoONE
Copy link
Contributor

Now searches with a depth of infinity from the file path, as described
in #66.

For my personal use, just switching the depth from 0 to the default (INFINITY) was enough; but as per the ticket I try to use the filePath now as well.

Now searches with a depth of infinity from the file path, as described
in vuejs#66.
@TheCycoONE
Copy link
Contributor Author

The issue is that the test is being run in vue-jest/temp-dir where the .babelrc is being renamed; but the search finds the babelrc in vue-jest now.

@eddyerburgh
Copy link
Member

Thanks for the PR.

What's your use case?

@TheCycoONE
Copy link
Contributor Author

TheCycoONE commented Jun 6, 2018

The ability to run jest from anywhere in the npm project, particularly the location of the spec file.

In particular this allows IntelliJ's Jest plugin to run without configuration in the IDE (by default IntelliJ sets the working directory to the path of the spec file when running tests from the editor) It is also consistent with babel-jest.

@eddyerburgh
Copy link
Member

We should keep the same functionality as babel-jest, but I don't think babel-jest searches in parent directories for the babel config file.

I'll have to take a deeper look at the source code. From what I could tell the babel options are found when the transformer is initially created, which I think is in the project root. We could implement something similar.

@TheCycoONE
Copy link
Contributor Author

My understanding is that this loop removes the last part of the path until it hits the root or escapes due to cache or finding the config. I have made an assumption about the filename being passed in, I could test that later if you like. https://github.com/facebook/jest/blob/4a46993b5116838a55666ac8c0522a71457eb7c5/packages/babel-jest/src/index.js#L37

@eddyerburgh
Copy link
Member

You're right, it does.

Ok, how about we rename the root babelrc as part of the test script, so it doesn't find it when the tests are running, then rename when the process exits?

@TheCycoONE
Copy link
Contributor Author

My feeling is that it is better to mock our findBabelConfig dependency so we aren't escaping the test sandbox, and the unit tests are more contained. I'm quite open to your feedback though

For tests that depend on there being no babelrc, rename the package one
now that we recurse.
@TheCycoONE
Copy link
Contributor Author

I made the test changes as you suggested.

@eddyerburgh
Copy link
Member

Hey, sorry that I've been silent on this. It slipped my radar.

I agree that we should add this feature, but we need a test for the filepath functionality specifically. Are you able to add a test to check that the babelrc searching uses the current directory?

@TheCycoONE
Copy link
Contributor Author

Sure, no problem. Likely tomorrow.

@TheCycoONE
Copy link
Contributor Author

@eddyerburgh Are those the tests you wanted?

@eddyerburgh
Copy link
Member

Thanks for adding the tests. I was thinking they should be tests that call through the whole process function, like these tests. The reason is that we will be refactoring the project soon and the load-babel-config function might be removed/ refactored. But they are fine as is. Thanks a lot for your patience and for creating this PR :)

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Thanks :)

@eddyerburgh eddyerburgh merged commit b40094f into vuejs:master Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants