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 babel injectable in babel-register #7273

Merged
merged 5 commits into from
Jan 26, 2018

Conversation

Janpot
Copy link
Contributor

@Janpot Janpot commented Jan 25, 2018

Q                       A
Fixed Issues? closes #7263
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? yes
Tests Added + Pass? Yes
Documentation PR no
Any Dependency Changes? no
License MIT

I made the babel instance used by babel-register configurable to allow for the use case in #7263.

@babel-bot
Copy link
Collaborator

babel-bot commented Jan 25, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6724/

@xtuc xtuc added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Jan 26, 2018
@xtuc
Copy link
Member

xtuc commented Jan 26, 2018

Could you please update the documentation of babel-register?

@Janpot
Copy link
Contributor Author

Janpot commented Jan 26, 2018

@xtuc ofcourse, my bad. I updated the PR.

cache: true,

// Specify the version of babel-core used for transformation. (Optional)
babel: require('babel-core')
Copy link
Member

Choose a reason for hiding this comment

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

require("@babel/core")?

Copy link
Contributor Author

@Janpot Janpot Jan 26, 2018

Choose a reason for hiding this comment

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

@nicolo-ribaudo Imagine calling babel-register from a location where a different version of @babel/core is available than the one that is depended on by babel-register

Copy link
Member

@xtuc xtuc Jan 26, 2018

Choose a reason for hiding this comment

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

That's a good point. @babel/register should also work with @babel/core, same for babel-register and babel-core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, in my case, it was using @babel/core but a conflicting version.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please change babel-core to @babel/core, then I think we're ready to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cache: true,

// Specify the version of babel-core used for transformation. (Optional)
babel: require('babel-core')
Copy link
Member

Choose a reason for hiding this comment

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

Could you please change babel-core to @babel/core, then I think we're ready to merge

@xtuc
Copy link
Member

xtuc commented Jan 26, 2018

Thanks for your PR @Janpot.

I'll go ahead and merge this, tests are green and it doesn't have a big impact anyway.

@xtuc xtuc merged commit 967414d into babel:master Jan 26, 2018
@Janpot Janpot deleted the babel-register-inject branch January 26, 2018 16:53
@loganfsmyth
Copy link
Member

This seems okay as a short-term fix, but it seems like ideally we'd suggest users use multiple versions of babel-register instead? Multiple babel-core versions also mean you need to either pass babelrc: false or explicitly limit compilation to certain subfolders where the settings only apply to that version of Babel.

This is also super dangerous because babel-register is a singleton and currently merges options instead of replacing them, so you can get arguments coming from multiple places. That's probably something I'd like to change though.

@xtuc
Copy link
Member

xtuc commented Jan 26, 2018

Yes, I agree, on the other hand, this allows to tight together babel-register and babel-core versions.

I don't know enough babel-register, but It seems to have quite some logic, i'm wondering if we could configure it through babel-core (by using the instance in argument).

@Janpot
Copy link
Contributor Author

Janpot commented Jan 26, 2018

In my use case I want to be able to register babel in a plugin to require a module from the user's project after which I will immediately deregister it to continue normal operation. I want to register the exact babel that is passed to the plugin.

@loganfsmyth
Copy link
Member

@Janpot That's the issue though, there is no "just register it and then undo it". Say for instance the user has their own script doing

require("babel-register")({
  only: ["./src"],
});

and then later in your process your logic kicks off with

require("babel-register")({
  babel: require("@babel/core"),
});

// do stuff

require("babel-register").revert();

That .revert() is also going to remove the user's configured build logic for compiling ./src. There is no API to register something and then undo the effects later currently. There probably should be, but it's not there now.

Personally while I appreciate the goal of this, I think it would be entirely unsafe to use in any realworld code, so I'd personally say we should revert this.

If the user expects to compile macros, it should either be up to them to add babel-register with

require("babel-register")({
   extensions: [".macro"]
});

somewhere, or it should be up to babel-plugin-macros to load the macros through a mechanism other than standard require, so it could expose its own way to compile macros.

Generally we don't recommend publishing code and expecting it to be processed with Babel afterward, so personally it sounds like this usecase would be addressed by precompiling the macro before it was published anyway.

@xtuc
Copy link
Member

xtuc commented Jan 26, 2018

Sorry about that it's my bad, let's revert this.

@Janpot
Copy link
Contributor Author

Janpot commented Jan 26, 2018

Generally we don't recommend publishing code and expecting it to be processed with Babel afterward, so personally it sounds like this usecase would be addressed by precompiling the macro before it was published anyway.

Well that's the thing, this wouldn't be used for published code at all since babel-register would ignore node_modules.

Also the .revert() shouldn't undo anything. This all happens on user code, that is already amongst other transpiled code.

@loganfsmyth
Copy link
Member

Well that's the thing, this wouldn't be used for published code at all since babel-register would ignore node_modules.

I thought the motivation for this PR was to use the babel-core version from a different module? In what cases do you need two babel versions in the same project?

Also the .revert() shouldn't undo anything. This all happens on user code, that is already amongst other transpiled code.

I'm not sure I follow, but I think I just don't have quite enough information. What do you mean it's all among transpiled code?

@Janpot
Copy link
Contributor Author

Janpot commented Jan 28, 2018

@loganfsmyth Ok, so the problem arises in the new create-react-app (v2). They're adding a macro system through a babel plugin babel-plugin-macros. While testing it out and playing with it I cam across two edge-cases:

  1. I tried to write a macro which was to be used as a building block for other macros's
  2. I tried to write a macro that is not published to npm but just lives locally to my code. As just another javascript file. maybe even sharing code with other parts of the project, isomorphic macro's let's say 🙂 .

Turns out this wasn't really possible. within the plugin, after the macro file is identified, it is just plain require-ed. So it turns out that in a folder in my CRA app, which is completely babel transpiled I could have

import myMacro from './my.macro';

It would be used as a macro, but it wouldn't be evaluated as an ES6 module. It wouldn't be evaluated with the same transpiler as all the surrounding code. I think this goes against expectations.

Also, since it wasn't transpiled, using a macro inside a macro is impossible and sharing code between the macro and the rest of the project as well, unless I wrote everything I want to share in commonjs style. (Or precompile it, or anything else that would defeat the use of convenience tools like CRA, that are supposed to work batteries-included, out-of-the-box)

So solution to both of my problems: transpile imported modules on the fly when executing the macro. Surrounding the require with @babel/register seems the logical solution. If the macro is an npm installed module, babel will ignore it, if it's a local module, it will be transpiled with the .babelrc applying to the location of that file.

Now when trying out this solution, I could make it work in one of my local folders without CRA, but when testing whether it would work in a CRA app, it gave errors that it didn't recognize certain (newer) AST node types. It seemed like it was trying to use babel-core that got installed in my top-level node_modules instead of the one bundled with CRA. Hence the need for specifying the @babel/core that is being used in @babel/register. I want to pass it the babel that is passed to the plugin, instead of finding one in my installed modules.

I hope with this background, my request makes a bit more sense. To me, your comment about the revert makes a lot of sense, it seems like maybe we rather need a

const babel = require('@babel/core');
const x = babel.transformRequire('./some-to-transpile-file.js');

or a

const requireWithBabel = require('@babel/register/require');
const x = requireWithBabel('./some-to-transpile-file.js', {
  babel: require('@babel/core')
});

Or it could be interesting to explore if we can create a context to be used with node vm module

const createContext = require('@babel/register/context');
const vm = require('vm');
vm.runInContext(`
  import * from './some-to-transpile-file.js';
`, createContext(require('@babel/core')));

?
Which in itself would probably not be a bad refactoring in an effort to take out as much side-effects code out of @babel/register itself.

@loganfsmyth
Copy link
Member

I'm still for reverting this, but I also do want to try to work to make sure we nail down the issue here. If you haven't ejected, I admit I'm not an expert, but is there way to load babel-register here in the first place? I don't think so, right? If you are doing it in here, you should be able to just install whatever version you want for @babel/register and @babel/core.

If you've ejected your CRA setup with Babel 6, you should be able to install babel-register to match whichever version of Babel CRA has installed in your project already. Ejecting CRA pretty much locks you into the Babel version that they support.

Maybe the issue here is that you're trying to use Babel 7 when CRA is still Babel 6?

To me, your comment about the revert makes a lot of sense, it seems like maybe we rather need

I do think there are a decent number of possible improvements to be made on the API.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: register PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the babel instance configurable in babel-register
6 participants