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

Allow preset-env to toggle module handling based on flags from the caller (like babel-loader) #8485

Merged
merged 6 commits into from Aug 20, 2018

Conversation

loganfsmyth
Copy link
Member

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

What this PR essentially means is that utilities calling Babel can:

  • Pass a name to Babel, so that if there are errors, it can give more useful error messages
  • Pass misc flags that presets can hook into

For instance, babel-loader would be able to do:

babel.transform("code;", {
  filename,
  presets: ["@babel/preset-env"],
  caller: {
    name: "babel-loader",
    supportsStaticESM: true,
  },
});

and preset-env can automatically change its behavior from modules: "commonjs" to modules: false.

@babel-bot
Copy link
Collaborator

babel-bot commented Aug 17, 2018

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

@babel-bot
Copy link
Collaborator

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

@loganfsmyth loganfsmyth added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Aug 17, 2018
@loganfsmyth
Copy link
Member Author

@Andarist Any thoughts on this, since it'll probably fit nicely into your work on the Rollup plugin too.

// When the `exclude` callback returns a truthy value, Babel not use the
// 'item' plugin here.
overrides.push({
exclude: (filename, { caller }) => caller && caller.supportsStaticESM,
Copy link
Member

Choose a reason for hiding this comment

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

Since the caller isn't file-specific but it is more similar to env, can we pass it using the api object instead using the exclude option (which is usally used to exclude plugins based on the file path)?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great question. Let me revisit that. This approach was from an old branch I had, so I hadn't acutally revisited that. My concern would be caching of plugin instances. The issue with caching is that the API would need to be

const supportsStaticESM = api.caller(caller => !!(caller && caller.supportsStaticESM));

because your build might be used both with and without static ESM support, and Babel needs to know whether or not it should re-evaluate the plugin function, since it does its best to only evaluate the plugin/preset functions a single time unless told otherwise.

I'm actually fine if we want to expose the api approach, but I'd still probably be tempted to leave this exposed in test/include/exclude too.

Copy link
Member

Choose a reason for hiding this comment

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

Caller is an object that can only contain null, undefined, booleans, strings, and numbers: it can be JSON.stringifyed and stored to invalidate the cache when needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, I'm personally not a huge fan of that because we could always want to expand that in the future, and having to re-stringify the object all the time just seems undesirable. At the end of the day I don't feel that strongly about it I guess. One other downside being that the plugin cache would get invalidated if any caller flag changed, even if it was something other than supportsStaticESM.

export function buildPresetChain(
arg: PresetInstance,
context: *,
): ConfigChain | null {
Copy link
Member

Choose a reason for hiding this comment

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

This function never returns null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good catch.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Aug 17, 2018

Does Babel use the same config validation for config files and options directly passed to babel.transform? If not, we could disallow caller in .babelrc/babel.config.js since users should directly configure the preset.

@loganfsmyth
Copy link
Member Author

If not, we could disallow caller in .babelrc/babel.config.js since users should directly configure the preset.

Already done! caller is only allowed as part of the programmatic call options to Babel.

moduleTransformations[modules] &&
// TODO: Remove the 'api.caller' check eventually. Just here to prevent
// unnecessary breakage in the short term for users on older betas/RCs.
(modules !== "auto" || !api.caller || !api.caller(supportsStaticESM))
Copy link
Member Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo Thoughts? Ended up exposing both and using the api.caller approach I mentioned.

});

it("`false` option returns commonjs", () => {
it("`false` option returns false", () => {
Copy link
Member

Choose a reason for hiding this comment

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

🤦‍♂️

@loganfsmyth loganfsmyth merged commit 2a4f162 into babel:master Aug 20, 2018
@loganfsmyth loganfsmyth deleted the caller-option branch August 20, 2018 17:44
@hzoo
Copy link
Member

hzoo commented Aug 20, 2018

This is really great! Something we should document for our integrations to also take advantage of.

@Andarist
Copy link
Member

🚀 super! Sorry I've missed this PR before, but this is super awesome - preventing the users misconfiguring their tool in clean way, gonna land this in rollup-plugin-babel once it gets released

abernix added a commit to abernix/hexo-renderer-react-emotion that referenced this pull request Aug 23, 2018
This resolves a frustrating error I encountered where I was receiving:

    ReferenceError: Unknown option: .caller

...when trying to use `@babel/register`.  This mysteriously went away when I
upgraded to `rc.2`, which was inspired after I honed in on related work to
the `caller` option in babel/babel#8485 after debugging
Babel's `loadOptions` (in `7.0.0-rc.1`) with the Node.js debugger.
` - 'false' to indicate no module processing\n` +
` - a specific module type: 'commonjs', 'amd', 'umd', 'systemjs'` +
` - 'auto' (default) which will automatically select 'false' if the current\n` +
` process is known to support ES module syntax, or "commonjs" otherwise\n`,

Choose a reason for hiding this comment

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

Hi, I have a ❓ about "if the current process is known to support ES module syntax". I'm not sure what that means. Can someone please clarify? Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

For example, if you are calling Babel using babel-loader, modules will be set to false because webpack supports ES modules

Choose a reason for hiding this comment

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

Thanks @nicolo-ribaudo, I'll link to your reply from babel/babel-loader#521 😃

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
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 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.

Should we leave the module transform enabled by default for env and es2015 in 7.x?
7 participants