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

Make sure .babelrc is a file, not a directory #427

Merged
merged 1 commit into from
Apr 21, 2017

Conversation

privatenumber
Copy link
Contributor

Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
If there is a directory called .babelrc in the repo root or it's parent, the build breaks with an error: Module build failed: Error: EISDIR: illegal operation on a directory, read stemming from find (/Users/hiroki/Documents/Gits/babel-loader/lib/resolve-rc.js:21:12)

What is the new behavior?
If the detected .babelrc is not a file, it ignores it and moves on.

Does this PR introduce a breaking change?

  • Yes
  • No

If this PR contains a breaking change, please describe the following...

  • Impact:
  • Migration path for existing applications:
  • Github Issue(s) this is regarding:

Other information:
There is logic in babel-core in build-config-chain.js that checks if .babelrc exists but neglects to check if it's a file. I will make a PR fixing it there as well.

@codecov
Copy link

codecov bot commented Mar 23, 2017

Codecov Report

Merging #427 into 7.0 will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##              7.0     #427   +/-   ##
=======================================
  Coverage   83.23%   83.23%           
=======================================
  Files           6        6           
  Lines         173      173           
  Branches       41       41           
=======================================
  Hits          144      144           
  Misses         12       12           
  Partials       17       17
Impacted Files Coverage Δ
src/utils/exists.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed8711d...9bccbbb. Read the comment docs.

@loganfsmyth
Copy link
Member

We can certainly throw a better error, but I don't think silently ignoring it makes sense?

@privatenumber
Copy link
Contributor Author

If there's a directory that is coincidentally named .babelrc in it's parent, that has nothing to do with the build (or perhaps not even Babel), does it make sense to throw an error that will prevent the build from happening?

That is the case I am currently encountering. When executing Babel on a shared server I do not have admin control in, it fails because someone created a .babelrc folder.

I think the particular part of the code I updated is not only concerned with whether .babelrc exists, but more so that it's readable.

@hiroppy
Copy link
Member

hiroppy commented Mar 24, 2017

babel/babel#5535

@loganfsmyth
Copy link
Member

Hmm, that's a fair point, if a little gross. I guess I can see the utility of this.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants