Use `babel`'s built-in option manager for loading configs. #28

izaakschroeder · 2016-11-02T10:34:27Z

There is an issue if you configure this plugin anywhere outside of your .babelrc. For example, if you have:

{
  "presets": ["my-preset"]
}

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.™

codecov-io · 2016-11-02T10:43:29Z

Current coverage is 100% (diff: 100%)

Merging #28 into master will increase coverage by 2.56%

Diff Coverage	File Path
•••••••••• 100%	src/index.js

Powered by Codecov. Last update 60aec31...c618af7

tleunen · 2016-11-02T10:52:50Z

Hi @izaakschroeder, thanks for the contribution.
Does it mean you also have the issue with babel-plugin-module-resolver?

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.

izaakschroeder · 2016-11-02T10:55:29Z

I don't think there is an issue with babel-plugin-module-resolver because it gets its config straight from babel itself (no config parsing shenanigans). This is the only package that needs updating and indeed this now makes it so both places get (roughly) the same config. There's extra logic in here for appending root and merging alias though (as it was before).

I don't think there's a better way unless babel decides to make them a more public API. The darker hackery was only to set cwd properly. I think this is workable in the meantime though for all the benefits it brings; you?

tleunen · 2016-11-02T11:00:00Z

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

izaakschroeder · 2016-11-02T11:03:49Z

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 .babelrc as the cwd (hence the [0] in the list of configs).

izaakschroeder · 2016-11-02T11:16:51Z

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's the latter, otherwise this whole thing wouldn't work 😄

tleunen · 2016-11-02T11:16:51Z

hmm At the same time, I'm not sure taking the first one as cwd is best too.

Let's say a project has /client/src and /server/src. You might have a root config at / with common usage then a more specific one in both configuration. If you have the module-resolver plugin set in /server, I believe the paths set in the configuration must be relative to /server, not /, right?

tleunen · 2016-11-02T11:17:55Z

It's the latter, otherwise this whole thing wouldn't work

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).

izaakschroeder · 2016-11-02T11:19:21Z

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 node_modules). The only thing would be if you had some module that you depended on that you wanted to alias. Seems like you could get around it by just using absolute paths in that case though.

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

izaakschroeder · 2016-11-02T11:27:42Z

You can see both builds passing using this branch. 😄

izaakschroeder · 2016-11-02T11:38:51Z

Let's say a project has /client/src and /server/src. You might have a root config at / with common usage then a more specific one in both configuration. If you have the module-resolver plugin set in /server, I believe the paths set in the configuration must be relative to /server, not /, right?

Correct. But we're making assumptions to begin with already which is that wherever we find a .babelrc relative to the original file is where we want your cwd to be (which would indeed be /server in this case in your original code too). Don't really think there is a "right" answer. If you want to be less opinionated then you should probably just make it a config option. In babel they pass in moduleRoot and sourceRoot and you can set that on the CLI too I think.

izaakschroeder · 2016-11-02T11:48:25Z

If you wanted to use that approach then you would need to pass that option through to babel here: https://github.com/babel/babel-eslint/blob/master/index.js#L368

tleunen · 2016-11-02T12:29:05Z

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

izaakschroeder · 2016-11-02T18:50:24Z

I always have 1 babelrc file at the root of the project so I'm not really affected by all this

Pretty much me too, at least as far as multiple .bablerc files go. My use case is for being able to use this in presets 😄

tleunen · 2016-11-04T13:33:21Z

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 :)

izaakschroeder · 2016-11-07T22:00:28Z

@tleunen if you're specifying cwd in the configuration, can we resolve this? (i.e. have it merged and use the cwd from the configuration).

tleunen · 2016-11-07T22:01:44Z

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

izaakschroeder · 2016-11-07T22:02:17Z

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.

izaakschroeder · 2016-11-07T22:09:57Z

@tleunen Thoughts on changing the default cwd option to:

pluginOpts.cwd || process.cwd()

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).

izaakschroeder · 2016-11-07T22:34:46Z

@tleunen Tests currently failing, but you can see the idea and there's a pretty big reduction in babel core voodoo for doing it.

loganfsmyth · 2016-11-07T23:07:55Z

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?

izaakschroeder · 2016-11-07T23:54:14Z

@loganfsmyth @tleunen Tests are now in place to notify users in case babel's internals get changed.

loganfsmyth · 2016-11-07T23:58:33Z

test/index.test.js

@@ -4,6 +4,7 @@

 const path = require('path');
 const resolverPlugin = require('../src/index');
+const OptionManager = require('babel-core/lib/transformation/file/options/option-manager');


This require could also fail since the file could get moved.

Resolved. 🚀

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/

izaakschroeder · 2016-11-08T00:17:34Z

Will squash when everyone is happy 🎉

loganfsmyth · 2016-11-08T04:45:31Z

src/index.js

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


Looking at this again, you can actually use require('babel-core').OptionManager.

👌 👌 👌

tleunen

This looks good, thanks @izaakschroeder for the recent changes!

And thanks a lot @loganfsmyth for the review

tleunen · 2016-11-08T13:25:31Z

package.json

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


Please revert to the 2.3.0 version, I'm dropping the v3 beta for now.

tleunen · 2016-11-08T13:26:41Z

src/index.js

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


not: Should be safe to move it again at the top of the file, right?

izaakschroeder · 2016-11-08T19:45:27Z

@tleunen Look good?

tleunen · 2016-11-09T02:10:03Z

Thank you both of you :)

vdh · 2016-11-10T05:57:21Z

This change caused some sort of disparity between local and global eslint, and maybe it's just this summer heat but I can't figure out what exactly is going on with this plugin's current directory handling.

tleunen · 2016-11-11T00:57:54Z

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

izaakschroeder force-pushed the use-babel-configurator branch from a22a82d to 153f3da Compare November 2, 2016 10:40

izaakschroeder force-pushed the use-babel-configurator branch from 153f3da to 64bde6d Compare November 7, 2016 22:07

izaakschroeder added 2 commits November 7, 2016 14:29

wip thing.

4dec959

fix thing.

8a88194

this will go.

877989c

Survive babel death.

358419c

loganfsmyth reviewed Nov 7, 2016

View reviewed changes

Jiggly pokey the internals to guard more aggressively.

9c57800

loganfsmyth reviewed Nov 8, 2016

View reviewed changes

Use straight from babel-core.

6e859f5

tleunen reviewed Nov 8, 2016

View reviewed changes

more feedbacks

c618af7

tleunen merged commit 952863e into tleunen:master Nov 9, 2016

izaakschroeder deleted the use-babel-configurator branch November 9, 2016 09:02

tleunen mentioned this pull request Nov 11, 2016

Revert "Use babel's built-in option manager for loading configs." #31

Merged

tleunen mentioned this pull request Dec 23, 2016

Use babel's built-in option manager for loading configs #34

Merged

Use babel's built-in option manager for loading configs. #28

Use babel's built-in option manager for loading configs. #28

Conversation

izaakschroeder commented Nov 2, 2016 • edited Loading

codecov-io commented Nov 2, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

tleunen commented Nov 2, 2016

izaakschroeder commented Nov 2, 2016

tleunen commented Nov 2, 2016 • edited Loading

izaakschroeder commented Nov 2, 2016

izaakschroeder commented Nov 2, 2016

tleunen commented Nov 2, 2016

tleunen commented Nov 2, 2016

izaakschroeder commented Nov 2, 2016 • edited Loading

izaakschroeder commented Nov 2, 2016

izaakschroeder commented Nov 2, 2016 • edited Loading

izaakschroeder commented Nov 2, 2016

tleunen commented Nov 2, 2016

izaakschroeder commented Nov 2, 2016

tleunen commented Nov 4, 2016

izaakschroeder commented Nov 7, 2016 • edited Loading

tleunen commented Nov 7, 2016

izaakschroeder commented Nov 7, 2016

izaakschroeder commented Nov 7, 2016 • edited Loading

izaakschroeder commented Nov 7, 2016

loganfsmyth commented Nov 7, 2016

izaakschroeder commented Nov 7, 2016

loganfsmyth Nov 7, 2016

Choose a reason for hiding this comment

izaakschroeder Nov 8, 2016

Choose a reason for hiding this comment

izaakschroeder Nov 8, 2016

Choose a reason for hiding this comment

izaakschroeder commented Nov 8, 2016

loganfsmyth Nov 8, 2016

Choose a reason for hiding this comment

izaakschroeder Nov 8, 2016

Choose a reason for hiding this comment

tleunen left a comment

Choose a reason for hiding this comment

tleunen Nov 8, 2016

Choose a reason for hiding this comment

tleunen Nov 8, 2016

Choose a reason for hiding this comment

izaakschroeder commented Nov 8, 2016

tleunen commented Nov 9, 2016

vdh commented Nov 10, 2016 • edited Loading

tleunen commented Nov 11, 2016

Use `babel`'s built-in option manager for loading configs. #28

Use `babel`'s built-in option manager for loading configs. #28

izaakschroeder commented Nov 2, 2016 •

edited

Loading

codecov-io commented Nov 2, 2016 •

edited

Loading

tleunen commented Nov 2, 2016 •

edited

Loading

izaakschroeder commented Nov 2, 2016 •

edited

Loading

izaakschroeder commented Nov 2, 2016 •

edited

Loading

izaakschroeder commented Nov 7, 2016 •

edited

Loading

izaakschroeder commented Nov 7, 2016 •

edited

Loading

vdh commented Nov 10, 2016 •

edited

Loading