Add useAutoDirectory option to transform-runtime plugin #8885

Andarist · 2018-10-16T21:27:38Z

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

Why?
When building libraries it's annoying to setup different options for @babel/plugin-transform-runtime for ESM and CJS builds.

With this the configuration can be unified with the newly introduced option and i.e. when bundling with rollup we could shorten this:

const config = [
  {
    input: 'src/index.js',
    output: { file: 'dist/library.js', output: 'cjs' },
    plugins: [
      babel({
        runtimeHelpers: true,
        plugins: [
          '@babel/transform-runtime'
        ]
      }),  
    ]
  },
  {
    input: 'src/index.js',
    output: { file: 'dist/library.esm.js', output: 'esm' },
    plugins: [
      babel({
        runtimeHelpers: true,
        plugins: [
          [
            '@babel/transform-runtime',
            { useESModules: true }
          ]
        ]
      }),  
    ]
  }
]

to this

const config =   {
  input: 'src/index.js',
  output: [
    { file: 'dist/library.js', output: 'cjs' },
    { file: 'dist/library.esm.js', output: 'esm' }
  ],
  plugins: [
    babel({
      runtimeHelpers: true,
      plugins: [
        [
          '@babel/transform-runtime',
          { useAutoDirectory: true }
        ]
      ]
    }),  
  ]
}

This technique of creating extra package.json files is not well-known but it's part of the node resolution algorithm - https://nodejs.org/api/modules.html#modules_all_together & resolution algorithms of webpack, rollup and other tools support this. This has an advantage of allowing tools to pick up appropriate files thanks to being able to specify both main & module entries for a single request path.

⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️
Unfortunately helpers written in CJS format have different shape from the helpers written in ESM. With ESM things are exported as default, with CJS they are assigned directly to module.exports.

As we know those things are not really compatible and I have 2 ideas how we could fix this (the issue to be fixed is that package.json in auto directory should point to modules with the same shape, regardless of the format):

introduce extra files 😭 that would reexport current CJS helpers as module.exports.default = require('../createClass') and point to those proxy files from auto package.json files
as helpers are mostly functions we can assign to them .default property that would self-reference to them, I'm not sure if this is completely safe though, could it cause problems with live bindings such as in extends helper?

babel-bot · 2018-10-16T21:31:32Z

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

loganfsmyth · 2018-10-17T04:15:24Z

packages/babel-plugin-transform-runtime/src/index.js

@@ -56,6 +57,14 @@ export default declare((api, options, dirname) => {
      "The 'useESModules' option must be undefined, or a boolean, or 'auto'.",
    );
  }
+  if (useESModules && useAutoDirectory) {
+    throw new Error("The 'useAutoDirectory' option must be a boolean.");


Looks like this message got swapped with the other error.

🤦‍♂️ fixed

loganfsmyth · 2018-10-17T04:16:38Z

packages/babel-plugin-transform-runtime/scripts/build-dist.js

+      filename,
+      JSON.stringify(
+        {
+          name: `${runtimeName}/helpers/auto/${helperName}`,


Are the name and private fields needed? I'd have expected only main and module.

They are probably not, Im just accustomed to using them. IMHO private gives an extra pinch of confidence that this won't be tried to get published (especially in lerna setup in case of some weird bug on their side).

name gives some more meaning to those files (kinda same with private, semantically both are ok). So I find adding those fields a nice touch, but as mentioned - they are not probably strictly required, so I can check it out and remove them if you feel they are not needed here.

My feeling is that if name isn't there, then an accidental publish isn't a concern either way. If we really want name, then I agree having the private: true is a good choice though. I'd probably rather leave them out and add them if we get reports of it being an issue.

Removed them.

Andarist · 2018-10-17T09:31:03Z

@loganfsmyth thanks for the comments! WDYT about the issue described in the "warning" section of the PR?

loganfsmyth · 2018-10-20T03:00:54Z

Hmm, I'd actually considered the current behavior to be more compatible since logically the default import of a CommonJS file is the module.exports object. For instance in Node's experimental mode, and Webpack, import helper from "@babel/runtime/helpers/classCallCheck"; would work correctly with either type of helper.

Does Rollup have different expectations in this case?

Andarist · 2018-10-20T09:28:57Z

The problem is that webpack became stricter (I think it has happened in v4) about module shapes and cjs/esm interop.

I've described the issue here - webpack/webpack#7973 .

loganfsmyth · 2018-10-24T01:23:40Z

Ah right. So I guess if we wanted to be as compliant and useable as possible, we'd want our CommonJS exports to change from

module.exports = function() {}

to something more like

function helper(){}
helper.default = helper;
helper.__esModule = true;
module.exports = helper;

which I think would allow it to work in all contexts alongside auto?

Andarist · 2018-10-24T07:24:18Z

Depends on if we consider manual (not generated by @babel/plugin-transform-runtime) imports to @babel/runtime allowed or not.

If we allow it then people might use non-.default require in their CJS code and thus make it incompatible for module-aware webpack consumers.

If we don't allow it - then yeah, it should work - because the only usage of those modules from CJS code would be inserted by babel itself and thus they should be imported with interop helper, right?

loganfsmyth · 2018-10-24T15:54:59Z

Depends on if we consider manual (not generated by @babel/plugin-transform-runtime) imports to @babel/runtime allowed or not.

I don't think I'd consider that valid, but I also wouldn't want to go out of my way to break it.

If we allow it then people might use non-.default require in their CJS code and thus make it incompatible for module-aware webpack consumers.

That would only break if they are using the auto imports though, I think?

Andarist · 2018-10-24T18:02:54Z

That would only break if they are using the auto imports though, I think?

If you are referring the other auto behaviour (based on supportsStaticESM) then - not sure 😅

When looking into the source of @babel/plugin-transform-runtime & @babel/helper-module-imports right now I see I was wrong - imports to those helper modules are inserted with importedInterop: "uncompiled" option, which from what I see means no interop helpers for those modules.

Let's look at possible scenarios - I'm narrowing it down only to libraries providing only CJS entry, because it's the only situation which is problematic.

default ({useESModules: false, useAutoDirectory: false})

var _classCallCheck = require("@babel/runtime/helpers/classCallCheck");

let Foo = function Foo() {
  "use strict";

  _classCallCheck(this, Foo);
};

Should be OK as @babel/runtime/helpers/classCallCheck provides only CJS export and webpack seeing __esModule on the exported object should just reach for .default which self-reference the helper

auto ({useESModules: "auto", useAutoDirectory: false})
Basically the same as previous case, I dont see right now how it would differ - but I could have missed smth.
auto directory ({useESModules: false, useAutoDirectory: true})

var _classCallCheck = require("@babel/runtime/helpers/auto/classCallCheck");

let Foo = function Foo() {
  "use strict";

  _classCallCheck(this, Foo);
};

I think this won't work because webpack will think that imported module has {default} shape and treat require(...) as a require to namespace object instead of a require to a single default export.

To sum it up I think the only way to support it at the moment is to introduce a separate strict directory which would contain files such as:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
module.exports.default = require('../createClass')

and point to that from auto directory. Ofc addDefault from @babel/helper-module-imports would have to be used with different mode (non-"uncompiled")

Andarist · 2018-11-15T14:24:16Z

@loganfsmyth any more thoughts on this? do you think this is too convoluted at this point in time to proceed with it?

Andarist · 2020-11-10T17:09:11Z

this is being superseded by #12295

Andarist added PR: New Feature 🚀 A type of pull request used for our changelog categories area: helpers labels Oct 16, 2018

Andarist mentioned this pull request Oct 16, 2018

Fix config being silently ignored in babel-plugin emotion-js/emotion#790

Merged

loganfsmyth reviewed Oct 17, 2018

View reviewed changes

Add useAutoDirectory option to transform-runtime plugin

53b6d49

Andarist force-pushed the feature/helpers-auto-directory branch from 2affdf7 to 53b6d49 Compare October 17, 2018 09:27

Remove name & private fields from created auto directories

2d64ad6

Andarist force-pushed the feature/helpers-auto-directory branch from 4c355aa to 2d64ad6 Compare October 19, 2018 08:04

Andarist mentioned this pull request Jan 10, 2019

Use @babel/runtime helpers preconstruct/preconstruct#4

Merged

Andarist closed this Nov 10, 2020

Andarist deleted the feature/helpers-auto-directory branch November 10, 2020 17:09

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 10, 2021

github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add useAutoDirectory option to transform-runtime plugin #8885

Add useAutoDirectory option to transform-runtime plugin #8885

Andarist commented Oct 16, 2018

babel-bot commented Oct 16, 2018 •

edited

loganfsmyth Oct 17, 2018

Andarist Oct 17, 2018

loganfsmyth Oct 17, 2018

Andarist Oct 17, 2018

loganfsmyth Oct 19, 2018

Andarist Oct 19, 2018

Andarist commented Oct 17, 2018

loganfsmyth commented Oct 20, 2018

Andarist commented Oct 20, 2018

loganfsmyth commented Oct 24, 2018

Andarist commented Oct 24, 2018

loganfsmyth commented Oct 24, 2018

Andarist commented Oct 24, 2018

Andarist commented Nov 15, 2018

Andarist commented Nov 10, 2020

Add useAutoDirectory option to transform-runtime plugin #8885

Add useAutoDirectory option to transform-runtime plugin #8885

Conversation

Andarist commented Oct 16, 2018

babel-bot commented Oct 16, 2018 • edited

loganfsmyth Oct 17, 2018

Choose a reason for hiding this comment

Andarist Oct 17, 2018

Choose a reason for hiding this comment

loganfsmyth Oct 17, 2018

Choose a reason for hiding this comment

Andarist Oct 17, 2018

Choose a reason for hiding this comment

loganfsmyth Oct 19, 2018

Choose a reason for hiding this comment

Andarist Oct 19, 2018

Choose a reason for hiding this comment

Andarist commented Oct 17, 2018

loganfsmyth commented Oct 20, 2018

Andarist commented Oct 20, 2018

loganfsmyth commented Oct 24, 2018

Andarist commented Oct 24, 2018

loganfsmyth commented Oct 24, 2018

Andarist commented Oct 24, 2018

Andarist commented Nov 15, 2018

Andarist commented Nov 10, 2020

babel-bot commented Oct 16, 2018 •

edited