-
Notifications
You must be signed in to change notification settings - Fork 31
Use babel
's built-in option manager for loading configs.
#28
Conversation
a22a82d
to
153f3da
Compare
Current coverage is 100% (diff: 100%)
|
Hi @izaakschroeder, thanks for the contribution. It's a good point you bring. with the plugin set in a preset, the current version won't find the configuration. I wonder if there's a better way other than using internal babel core functions though. |
I don't think there is an issue with I don't think there's a better way unless |
The beta version of the plugin tries finds the babelrc config location to determine the working directory for the paths resolution, so we might also have others issues I guess... (see this change) When you're setuping the plugin in a preset, what are the paths you're using? relative to the config or relative to the project root directory? If it's relative to the babelrc file, we should be fine |
It should be possible to change that to use a more refined resolution mechanism if it's needed. I think plugins have access to a bunch of information about themselves. The premise of this was to use the "original" or root |
It's the latter, otherwise this whole thing wouldn't work 😄 |
hmm At the same time, I'm not sure taking the first one as cwd is best too. Let's say a project has |
Yep after your previous comment I figured that out ;) But it could have been relative to the config specifying the preset (which could make more sense to me). |
I think it would be fairly pointless relative to the config... since there's no way of knowing where the config is mounted at (i.e. at which level within Results are here: https://github.com/metalabdesign/babel-preset-metalab/pull/12/files and here: https://github.com/metalabdesign/eslint-config-metalab/compare/use-new-resolver?expand=1 |
You can see both builds passing using this branch. 😄 |
Correct. But we're making assumptions to begin with already which is that wherever we find a |
If you wanted to use that approach then you would need to pass that option through to |
I always have 1 babelrc file at the root of the project so I'm not really affected by all this, I'm just trying to find the best solution to your use case and the ones from others users who don't have the babelrc at the root but in the /src directory for example |
Pretty much me too, at least as far as multiple |
How will it work when people specify the configuration using babel-register? (see tleunen/babel-plugin-module-resolver#61 for context) Also if @hzoo or @loganfsmyth could tell me if there's a better way instead of using internal babel stuff, it would be great :) |
@tleunen if you're specifying |
Yep, lets get this merged and we'll see later if we can have a better support from Babel instead of using internal stuff. There're conflicts now in the PR though |
Will rebase now 😄 |
There is an issue if you configure this plugin anywhere outside of your `.babelrc`. For example, if you have: ```json { "presets": ["my-preset"] } ``` And in your preset you have this plugin configured, then this plugin will fail to find that configuration value. By leveraging babel's built-in option manager for doing all the heavy lifting we avoid these pitfalls.
153f3da
to
64bde6d
Compare
@tleunen Thoughts on changing the default
Which is much easier to reason about and the user can set it to whatever they want and avoids this scary problem of walking the babel configuration tree (and means I can use less internal functions). |
@tleunen Tests currently failing, but you can see the idea and there's a pretty big reduction in babel core voodoo for doing it. |
There is no official API for this and I'm not sure if that's something I'd want to add, but either way if you're going to depend on this could I at least recommend wrapping these in a try/catch with a useful error message, so if it one day breaks because Babel changes something, users will know it is because of this module, not something Babel did? |
@loganfsmyth @tleunen Tests are now in place to notify users in case |
@@ -4,6 +4,7 @@ | |||
|
|||
const path = require('path'); | |||
const resolverPlugin = require('../src/index'); | |||
const OptionManager = require('babel-core/lib/transformation/file/options/option-manager'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This require
could also fail since the file could get moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved. 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather, resolved in the module code itself. This file is just a test so I don't think guards are necessary here. And indeed, I'm sure this issue could be caught quickly with this present + https://greenkeeper.io/
Will squash when everyone is happy 🎉 |
@@ -49,6 +15,30 @@ function opts(file, config) { | |||
|
|||
exports.interfaceVersion = 2; | |||
|
|||
function getPlugins(file, target) { | |||
const OptionManager = require('babel-core/lib/transformation/file/options/option-manager'); // eslint-disable-line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again, you can actually use require('babel-core').OptionManager
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 👌 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks @izaakschroeder for the recent changes!
And thanks a lot @loganfsmyth for the review
@@ -43,7 +43,8 @@ | |||
"standard-version": "^3.0.0" | |||
}, | |||
"peerDependencies": { | |||
"babel-plugin-module-resolver": "^2.3.0" | |||
"babel-core": "^6.0.0", | |||
"babel-plugin-module-resolver": "^3.0.0-beta" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert to the 2.3.0 version, I'm dropping the v3 beta for now.
@@ -49,6 +15,30 @@ function opts(file, config) { | |||
|
|||
exports.interfaceVersion = 2; | |||
|
|||
function getPlugins(file, target) { | |||
try { | |||
const OptionManager = require('babel-core').OptionManager; // eslint-disable-line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not: Should be safe to move it again at the top of the file, right?
@tleunen Look good? |
Thank you both of you :) |
This change caused some sort of disparity between local and global |
I have, hopefully, temporarily reverted the changes and released v2.2.1. I'll check more over the weekend how to fix the cwd issue. See #30 |
There is an issue if you configure this plugin anywhere outside of your
.babelrc
. For example, if you have:And in your preset you have this plugin configured, then this plugin will fail to find that configuration value. It will also fail if you have multiple
.babelrc
's and the configuration is defined up the tree somewhere (as is also possible).By leveraging babel's built-in option manager for doing all the heavy lifting we avoid these pitfalls. No dealing with environment settings or checking the shape of the configuration object. It's also a lot less code and coverage goes to the moon 🚀 .
The only minor concern is that it leverages a bit of babel's "inner" workings and if they change that then this could break but that's no better than how it is now really.
Everything. Just. Works.™