Have @babel/core lazy-load all dependencies and make @babel/register not explode because of that by loganfsmyth · Pull Request #7588 · babel/babel

loganfsmyth · 2018-03-17T21:48:29Z

Q	A
Fixed Issues?	Fixes #5702 (but not primary goal)
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?
License	MIT

There is some serious craziness in here for @babel/register sooooo sorry 😬 Essentially we want to be able to load babel/core with lazy requires safely in @babel/register, and that's dangerous/broken if register were left as-is because calling babel.transform could then actually trigger the require hook again causing a cycle.

Triggering a cycle isn't guaranteed to be an issue, since you could imagine a world where someone does:

// .babelrc
{
  presets: ['@babel/env'],
  overrides: [{
    exclude: "./my-custom-plugin",
    plugins: [
      "./my-custom-plugin",
    ],
  }],
}

to compile a project with some custom plugin, while also compiling that plugin itself with a different config.

To achieve all that while still having @babel/core kick off require calls at random times during transformation, this PR makes @babel/register much smarter about how it loads @babel/core. You can almost think of it like it loads it with its own loader context that explicitly tracks while files it has loaded. By tracking all that, we can explicitly skip those files in the require hook, avoiding trying to compile files that are necessary for the compilation itself.

There is then a small hole exposed in that loader logic so that when @babel/core loads .babelrc.jss, plugins, and presets, it will not track those, so that they actually do get passed to the require hook. This means it is up to the user to ensure that their Babel config does not introduce dependencies. We also have existing logic in @babel/core to show helpful error messages when there are cycles detected in config files/plugins/presets.

The end result of this PR is that @babel/core can lazy-load everything and all the rest of babel-register pretty much ends up behaving like it did before.

Why!

The end goal of this PR is the lazy-loading. With PR's like #7472 encouraging tooling to load @babel/core to check for config, this means we need to make loading core as fast as possible. Currently, it is the exact opposite of that. A single call to require("@babel/core") on my machine for instance takes ~250-350ms. With this PR, it now takes ~25-30ms on my machine.

This brings down the total time for a call to require("@babel/core").loadPartialConfig() dramatically, which should make it easier for people to adopt over trying to implement the logic themselves.

babel-bot · 2018-03-20T05:09:42Z

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

babel-bot · 2018-03-20T05:09:42Z

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

Jessidhia · 2018-03-20T05:29:27Z

packages/babel-register/src/babel-core/track-deps.js

+  }
+
+  // Ensure that require.resolve and other properties are carried over.
+  return Object.assign(babelRegisterRequire, req);


They're enumerable? :o

Seems like!

Jessidhia · 2018-03-20T05:31:31Z

packages/babel-register/src/babel-core/track-deps.js

+    __filename,
+  )}).wrap(require, __filename, ${uid}, ${inRoot})`;
+
+  return `return (function(require){${code}\n}).call(this, ${override});`;


top level return is icky 😅

Yeah for some reason I was thinking it was needed, but it isn't is it.

wonder how this affects the error messages (although that might just be messed up already)

They'll be an extra scope in the debugger if you pause, but otherwise I don't think it will affect anything.

Jessidhia · 2018-03-20T05:37:51Z

packages/babel-register/src/babel-core/track-deps.js

+    inRoot: boolean,
+  ) => { exports: mixed } | void,
+) {
+  const files = new Set();


The set seems to be write-only?

It ends up exported and used in /node.js to skip them.

hzoo · 2018-03-23T18:41:37Z

Were you able to test this? should probably run locally before next release then like #6134. For compiling with lazy and also new register although I guess this is a fix for the previous time we tried this.

loganfsmyth · 2018-03-23T20:06:31Z

The tests for babel-node fail without this logic in babel-register so I'm relatively confident that it works well.

The main question for me right now, which I'm going back and forth on, is whether we care about the case of re-entrant configs. And if we do (which I think I do?), should it be something we require users to opt into?

For instance right now people often do ignore: [] to have babel-register process every file, which because we allow re-entrant plugins, means that this triggers a cyclical error because loading the preset itself will trigger an error.

I think I'm personally leaning toward us making that behavior opt-in instead. We'd then instead require users to do

require("@babel/register")({
  reentrant: true
});

if they wanted register to compile a plugin/preset itself, before using that plugin/preset somewhere else in their build process.

It'd be a lot safer for the average user since we get issues like #7208 reasonably often.

Doing that would still require the logic in this PR, but I'd remove the overrideCallback entirely and instead we'd have to pass the actual plugin/config require callback to Babel itself during the call to .transform.

hzoo · 2018-03-24T13:20:50Z

Ok, just needs docs on that and maybe think if that's the best word for it. I guess not sure what else it would be called but the intention is to compile configs themselves with register which affect itself? Or could we error with a nicer message when it happens without?

loganfsmyth · 2018-03-24T17:35:44Z

Or could we error with a nicer message when it happens without?

I don't follow. If we error without the flag on you mean? Without the flag I don't think I'd expect it to error, and with the flag we already give some errors about reentrant code.

hzoo · 2018-03-24T17:36:48Z

just wondering how someone would know to use the flag, if it's clear then never mind

loganfsmyth · 2018-03-24T17:41:42Z

I think what I might do is tweak this PR to disallow reentrant plugins/presets entirely for now, and land that, and then do a second PR to add the reentrant option and the code that is necessary in babel-core to make it work.

loganfsmyth · 2018-03-24T17:43:02Z

just wondering how someone would know to use the flag, if it's clear then never mind

Gotcha, yeah it's not super clear, but then I don't think it'll be super common anyway. It's not something that'll work with any of the other Babel integrations anyway since they don't do require hooks, so I think making it a special opt-in feature of babel-register seems fine. We'll definitely have to document it clearly though.

loganfsmyth · 2018-03-25T21:32:08Z

Alright I decided to remove most of the complex logic that I had originally here for register in favor of totally disallowing re-entrant compilation. I added a note to the README on the recommended approach of you do want to compile a plugin up front, it should still cover 99% of the cases where people would actually want to live-compile a plugin/preset.

Nantris · 2018-04-28T22:20:02Z

@loganfsmyth; note in the README? Not sure where you're referring to or if this is relevant to the issue I'm having anyway (#7208), but if you could point me towards that note I'd appreciate it. Thanks for all your hard work!

loganfsmyth force-pushed the register-cycle-fixes branch 2 times, most recently from 6ff42db to 52dac67 Compare March 20, 2018 05:03

Jessidhia reviewed Mar 20, 2018

View reviewed changes

xtuc added the PR: Internal 🏠 A type of pull request used for our changelog categories label Mar 22, 2018

timneutkens mentioned this pull request Mar 23, 2018

Upgrade Babel 6 to Babel 7 (major) vercel/next.js#4050

Merged

hzoo approved these changes Mar 24, 2018

View reviewed changes

loganfsmyth added 4 commits March 25, 2018 12:50

Ensure that sourceMapSupport is installed in all branches.

9e4dd86

Include stdout in error messages to aid debugging.

40e3864

Have @babel/register explicitly skip re-entrant plugins/presets.

1dbed5f

Lazy-load @babel/core's dependencies to decrease up-front load times.

a5df709

loganfsmyth force-pushed the register-cycle-fixes branch from 52dac67 to a5df709 Compare March 25, 2018 21:26

loganfsmyth merged commit a10c917 into babel:master Mar 25, 2018

loganfsmyth deleted the register-cycle-fixes branch March 25, 2018 22:24

JLHwung mentioned this pull request Sep 15, 2019

perf: only apply lazy cjs module transform on cli and core #10443

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Uh oh!

Conversation

loganfsmyth commented Mar 17, 2018

Why!

Uh oh!

babel-bot commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzoo commented Mar 23, 2018

Uh oh!

loganfsmyth commented Mar 23, 2018

Uh oh!

hzoo commented Mar 24, 2018

Uh oh!

loganfsmyth commented Mar 24, 2018

Uh oh!

hzoo commented Mar 24, 2018

Uh oh!

loganfsmyth commented Mar 24, 2018

Uh oh!

loganfsmyth commented Mar 24, 2018

Uh oh!

loganfsmyth commented Mar 25, 2018

Uh oh!

Nantris commented Apr 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

babel-bot commented Mar 20, 2018 •

edited

Loading