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

Support 'packagejson' as a custom cwd option. #149

Merged
merged 4 commits into from
Apr 13, 2017

Conversation

cpsubrian
Copy link
Contributor

@cpsubrian cpsubrian commented Mar 30, 2017

Some context:

  • I'm building an app as a mono-repo with lerna.
  • I'd like to manage all the .babelrc config at the root level of the repo (not per sub-package).
  • I'd like to be able to do import/requires like: import db from 'src/db', where 'src' is relative to the root of the package of the file being transpiled.

This PR facilitates that and is working great for me so far.

Sample .babelrc configuration:

{
  "plugins": [
    ["babel-plugin-module-resolver", {
      "root": ["./"],
      "cwd": "packagejson"
    }]
  ]
}

The key is that this approach works not matter what your actual 'cwd' is. This means that my imports work whether I am running babel-node/babel-watch from the repo root, or running babel on each sub-package individually.

I'm not married to find-root, it was literally the first 'find my package.json' module that I found 😄

@fatfisz
Copy link
Contributor

fatfisz commented Apr 5, 2017

@cpsubrian Hey, thanks for your contribution and sorry for taking that long!

Two small things:

  • could you use pkg-up instead? It's also used in the "sibling" plugin eslint-import-resolver-babel-module, so the dependency would be shared.
  • could you change the two ifs into a switch? This way the second condition won't depend on the first one in all cases.

@cpsubrian
Copy link
Contributor Author

@fatfisz Thanks for the review. Will look into those changes and update.

@fatfisz
Copy link
Contributor

fatfisz commented Apr 10, 2017

@cpsubrian Hi, could you also rebase this on the beta branch?

The file where you should put the changes now is src/normalizeOptions.js, and the method is called normalizeCwd.

@cpsubrian
Copy link
Contributor Author

@fatfisz Yep, no problem. Will do when I get a moment. Had to jump over to a different project last week.

@codecov
Copy link

codecov bot commented Apr 12, 2017

Codecov Report

Merging #149 into beta will not change coverage.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/normalizeOptions.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 e9f9bf6...46b2155. Read the comment docs.

@cpsubrian cpsubrian changed the base branch from master to beta April 12, 2017 20:43
@cpsubrian
Copy link
Contributor Author

@fatfisz Rebased to beta and made those changes. I went with an else if instead of switch because the additional syntax noise not worth it unless we get to 3+ options (IMO). Technically, if we are going for micro-optimization and we expect most people to not use opts.cwd, then we should bail out on that case first (but then there will be some redundant logic in the function).

@fatfisz
Copy link
Contributor

fatfisz commented Apr 13, 2017

@cpsubrian That's ok, else if also works :) Will merge this later today.

@fatfisz
Copy link
Contributor

fatfisz commented Apr 13, 2017

Ooops, I've only now noticed a small problem in the tests - in two cases you are using describe instead of it. I'll just fix that before merging.

@fatfisz fatfisz merged commit 52600af into tleunen:beta Apr 13, 2017
@cpsubrian
Copy link
Contributor Author

@fatfisz Oh haha, I had fixed those locally but I guess I didn't push them. Oops!

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.

2 participants