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

@babel/plugin-transform-modules-amd doesn't behave as expected #9793

Open
jslegers opened this issue Mar 29, 2019 · 5 comments
Open

@babel/plugin-transform-modules-amd doesn't behave as expected #9793

jslegers opened this issue Mar 29, 2019 · 5 comments

Comments

@jslegers
Copy link

Bug Report

I have the following package.json :

{
  "name": "watchman",
  "version": "1.0.0",
  "description": "Simple watcher for ES6 to AMD conversion",
  "author": "Luciad NV",
  "license": "MIT",
  "scripts": {
    "build": "cross-env NODE_OPTIONS=--max_old_space_size=8192 babel es6/geometry es6/ria es6/symbology -d release --watch"
  },
  "devDependencies": {
    "@babel/core": "^7.4.0",
    "@babel/plugin-transform-modules-amd": "^7.2.0",
    "cross-env": "^5.2.0"
  },
  "dependencies": {
    "@babel/cli": "^7.2.3"
  }
}

I have the following .babelrc :

{
  "plugins": ["@babel/plugin-transform-modules-amd"]
}

I have the following es6 module in my es6/ria folder :

import ProgrammingError from "../error/ProgrammingError";
import Promise from "../util/Promise";

  function Evented(supportedEvents) {
  }

  Evented.prototype = Object.create(Object.prototype);
  Evented.prototype.constructor = Evented;

  Evented.prototype.on = function(event, callback, context, options) {
  };

  Evented.prototype.emit = function(event) {
  };

  export default Evented;

If I run npm run build This produces an AMD module with the following implementation :

define(["exports", "../error/ProgrammingError", "../util/Promise"], function (_exports, _ProgrammingError, _Promise) {
  "use strict";

  Object.defineProperty(_exports, "__esModule", {
    value: true
  });
  _exports.default = void 0;
  _ProgrammingError = _interopRequireDefault(_ProgrammingError);
  _Promise = _interopRequireDefault(_Promise);

  function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

  function Evented(supportedEvents) {}

  Evented.prototype = Object.create(Object.prototype);
  Evented.prototype.constructor = Evented;

  Evented.prototype.on = function (event, callback, context, options) {};

  Evented.prototype.emit = function (event) {};

  var _default = Evented;
  _exports.default = _default;
});

When I try to load the Evented module with eg. Require.Js, I expect the Evented "class".

Instead I get an object with a default property, which contains the Evented "class".

Is this a bug? Or is it designed this way?

And it it isn't a bug, is there a way to achieve the desired effect?

Is there a way I can convert this ES6 to AMD with the @babel/plugin-transform-modules-amd plugin, so it returns the Evented "class" as expected?

@babel-bot
Copy link
Collaborator

Hey @jslegers! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community
that typically always has someone willing to help. You can sign-up here
for an invite.

@jslegers jslegers changed the title @babel/plugin-transform-modules-amd produces incorrect AMD modules @babel/plugin-transform-modules-amd doesn't behave as expected Mar 29, 2019
@nicolo-ribaudo
Copy link
Member

There isn't: a default export is just a named export whose name is default. Otherwise, this code couldn't work:

export default { foo: 2 };
export const foo = 3;

@jslegers
Copy link
Author

jslegers commented Mar 29, 2019

Otherwise, this code couldn't work:

export default { foo: 2 };
export const foo = 3;

Why not?

What I would expect as output in that case, is for only the default export to be exported and for the named export to be ignored.

AMD doesn't know the concept of named & default exports, really, and trying to include both when you convert from ES6 to AMD therefore doesn't really make much sense... especially considering it doesn't allow converting from legacy AMD to ES6 and back.


Our use case

At Luciad, we are currently in the process of converting an AMD library to ES6, but we need to maintain backwards compatibility for all of our older customers who still use AMD, by converting our ES6 modules back to AMD and shipping it in our release, along with our ES6 version.

At the moment I'm using jonbretman's amd-to-es6 to convert from AMD to ES6. This will be a one time conversion. All exports will be default exports.

I'm using ecomfe's babel-plugin-transform-modules-amd to convert back to AMD. This conversion will occur at every code change until we can officially drop AMD support. Default exports should be converted & named exports should be ignored.

For most part, this two-way conversion seems to work as expected. However, the latter only supports either default or named exports. Instead of ignoring one of them or including both, it just throws a TypeError if I tried to add a named export next to the default export. This means that we won't be able to use it when we start adding named exports to our library in the future.

For maintainability sake I would prefer to use an official babel plugin, but with your current implementation this is not possible for us, as it offers no way to get the expected output.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 30, 2019

What I would expect as output in that case, is for only the default export to be exported and for the named export to be ignored

That's not how es modules work. AMD and CommonJS aren't compatible with ES Modules, and the .default trade-off is the best we could find.

Another example that doesn't work without the .default property:

// module.js

export default function foo() { return 2 }

foo.update = function () {
  foo = 3;
}
// main.js

import { default as x } from "./module.js"
console.log(x); // a function
x.update();
console.log(x); // 3

If you want to use @babel/transform-modules-amd, I suggest creating something similar to https://www.npmjs.com/package/babel-plugin-add-module-exports, which "hacks" the official plugin and removes the .default property.

I'm using ecomfe's babel-plugin-transform-modules-amd to convert back to AMD. This conversion will occur at every code change until we can officially drop AMD support.

Can't you just add it to your normal Babel config?

This means that we won't be able to use it when we start adding named exports to our library in the future.

Also not setting the default export using the .default property has this problem.

For most part, this two-way conversion seems to work as expected. However, the latter only supports either default or named exports. Instead of ignoring one of them or including both, it just throws a TypeError if I tried to add a named export next to the default export.

It is better to throw a TypeError at compile time, rather than it being ignored and then having bugs caused by missing exports (

What I would expect as output in that case, is for only the default export to be exported and for the named export to be ignored.

). Everything that can be reported at compile-time rather than at runtime should be reported, and that plugin is doing the correct thing.

@Aghassi
Copy link

Aghassi commented May 14, 2019

@nicolo-ribaudo is it possible to make it so that plugin accepts an argument that allows for what @jslegers is requesting? Our business is in a similar position and the ability to not have to refactor legacy code to make everything use .default is huge. We understand the risks involved, however we need a way forwards. Turning on an argument to allow that would be our way of saying "yes I know this isn't the way it is suppose to be used, but we accept the risk and want to do this". Any breakage from there is on us not you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants