Refactored the library to es6 modules, added babel build step for out…

[Andarist]

…putting commonjs files, using rollup to make UMD builds

fixes #1968

I'm a little hot headed and prepared a refactor based on this request.

This is a breaking change for cjs users, so migration/transition paths need to be established.

cc @jonaskello @kedashoe

[jonaskello]

Nice work :-)

[Andarist]

@jonaskello if you have any suggestions what could be changed to make it better, I would highly appreciate any input :) especially in regards of transitioning between versions

[kedashoe]

Thanks for the pr @Andarist ! Couple quick questions

1. do we need to break things for cjs?
2. do we need babel?

[Andarist]

do we need to break things for cjs?

I think we can preserve old behaviour by using custom cjs transform.

With the current setup such code:

import _curry2 from './internal/_curry2';

var add = _curry2(function add(a, b) {
  return Number(a) + Number(b);
});
export default add;

produces this for the commonjs format:

'use strict';
Object.defineProperty(exports, "__esModule", {
  value: true
});
var _curry = require('./internal/_curry2');
var _curry3 = _interopRequireDefault(_curry);
function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

var add = (0, _curry3.default)(function add(a, b) {
  return Number(a) + Number(b);
});
exports.default = add;

Therefore such module have to be consumed like this when using require - var add = require('ramda/cjs/add').default. There is no problem when requiring whole ramda, nothing changes there - var add = require('ramda').add.

Attempt to change those exports to named ones yield such result:

// same 'banner'
var add = exports.add = (0, _curry3.default)(function add(a, b) {
  return Number(a) + Number(b);
});

Even with other options passed to the transform like { loose: true, noInterop: true, strict: false } we cannot completely preserve old structure, because it assumes we want new structure (for better interoperability) by using es6 modules syntax.

Therefore I believe that if we want to support the old way just the way it is we need to:

• write a custom es modules transform (I can do it, should be fairly easy)
• move current /src to /es and transpile /es to /src

do we need babel?

We dont need it per se but id argue we want it, because working with code transformations on ASTs is way easier/cleaner than transforming bare files and its contents as string. It also opens immediately its whole ecosystem and can allow further refactors to es6 - most notably arrow functions which would be a perfect match for ramda's style :)

@kedashoe sorry for a wall of text, probably could have slim it down, but I wanted to share the exact reasoning. Please decide how I should proceed with this further

[jonaskello]

I recall using the add-module-exports plugin to fix the .default require issue.

[Andarist]

@jonaskello nice! I had a feeling this must be a solved problem, but couldnt find a plugin for this :)

with this setting:

// .babelrc
{
  "plugins": [
    "add-module-exports",
    ["transform-es2015-modules-commonjs", {"loose": true, "noInterop": true, "strict": false}]
  ]
}

we can go down to this

exports.__esModule = true;
var _curry = require('./internal/_curry2');

var add = (0, _curry.default)(function add(a, b) {
  return Number(a) + Number(b);
});
exports.default = add;
module.exports = exports['default'];

Its still slighly bigger than original though:

var _curry2 = require('./internal/_curry2');

module.exports = _curry2(function add(a, b) {
  return Number(a) + Number(b);
});

and you need to decide if thats acceptable or not :)

[kedashoe]

Thanks for the rec @jonaskello , sounds like it's worth a shot. Can you update the PR to use https://github.com/59naga/babel-plugin-add-module-exports, @Andarist ?

Also, what do you think about https://github.com/morlay/babel-plugin-annotate-pure-call-in-variable-declarator for automatically adding the PURE annotations?

Can we remove the acorn dependency now?

Is there a reason not to use buble? Seems to be the rollup recommended way to integrate babel?

[fix-fix]

Do we need a Makefile at all after these changes?

I don't think there is any reference to make in the source code or docs, and there were some issues with make on non-Unix systems (for example #1953).

/cc @davidchambers

[Andarist]

there is still release script left there, personally im not really fond of Makefiles, but i assumed its preferred by the Ramda's core team, so I have left it in place

[davidchambers]

I love makefiles, and use them in all my projects. I don't contribute to Ramda much any longer, though, so I'm happy for you people to decide what is best for the project.

I urge you to keep in mind, @kedashoe, that as you currently seem to be the primary maintainer of this project you will be most affected by the consequences—positive and negative—of any changes to the way Ramda is built and distributed.

[Andarist]

Thanks for the rec @jonaskello , sounds like it's worth a shot. Can you update the PR to use https://github.com/59naga/babel-plugin-add-module-exports, @Andarist ?

added

Also, what do you think about https://github.com/morlay/babel-plugin-annotate-pure-call-in-variable-declarator for automatically adding the PURE annotations?

oh, thats a newie! OTOH at the moment I transpile only once to cjs, adding this would require transpiling twice - to cjs target and es target (because source code, while written with es module, wouldnt have those comments)

the only problem I have with this is that I dont know how to call the new directory
lib - taken, some scripts in there
src - target for cjs, .gitignored, but published to npm
es - source code, in repo, and published to npm
dist - umd builds

Also at the moment I think this plugin is overdoing the transformation, because it annotates every variable declarators and I think (if im wrong, please correct me) its only needed for the top level calls (for children of the body - ast speaking), so it produces:

export const A = /*#__PURE__*/(() => {
  var B = /*#__PURE__*/call();
})();

That would increase quite significantly file sizes on the npm, however its easily fixable. Gonna raise an issue there.

Can we remove the acorn dependency now?

dropped

Is there a reason not to use buble? Seems to be the rollup recommended way to integrate babel?

At the moment building with rollup doesnt need any babel integration, because we do not transform anything else than es modules, and rollup already knows how to do that. It will be needed though if you decide to add any other transformations like arrow functions or others. Ive always done it with adding rollup-plugin-babel though. I believe buble is an alternative for babel which only supports es2015 transformations.

[Andarist]

Also while adding https://github.com/59naga/babel-plugin-add-module-exports helped with keeping the old style of requires, it interferes with transform-es2015-modules-commonjs because of that I couldnt use "noInterop": true for the latter - this results in

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

being included in each file that imports other module. That is because used imports are written like this:

(0, _imported.default)

Without interop _imported became module itself (cause of module.exports = exports['default'];) and had no .default property on it.

[kedashoe]

Sorry I'm not understanding where we are with babel. Up above I asked if we needed it and you said

We dont need it per se but id argue we want it, because working with code transformations on ASTs is way easier/cleaner than transforming bare files and its contents as string.

but asking about buble you say

At the moment building with rollup doesnt need any babel integration, because we do not transform anything else than es modules, and rollup already knows how to do that.

As to https://github.com/59naga/babel-plugin-add-module-exports, if that interferes with the exports, how about writing our own transform?

[Andarist]

@kedashoe
I'll try to clarify what I've meant :)

From what I understand buble is an alternative to babel. I thought u were suggesting using buble in combination with rollup, thinking it is a plugin for babel integration. Actually for a rollup (which creates UMD builds for us) at the moment any additional AST transformations are not needed, because the only thing we want to transpile for now are es modules which rollup handles on its own - without any additional tools.

In theory we could transform source files (containing es modules) 'by hand' to the commonjs format, because it should be quite trivial, in similar way like you build ur UMD files today.

However transformations on AST (what babel does) is a more high level concept than parsing source code as a string and less brittle, so I advise to use this full-blown solution (babel) even for this single (for now) transform we need to do.

As to https://github.com/59naga/babel-plugin-add-module-exports, if that interferes with the exports, how about writing our own transform?

Sure thing, I've proposed this as viable option before. Gonna make this in following days.

[Andarist]

@kedashoe I've written custom transformer quickly, which is heavily based on the original babel's transform. It supports ramda's needs nicely - doesnt cover each form of the import/export syntax, where I could Ive added checks which should throw against wrong usage (i.e. using named exports).

I've omitted some cases in the implementation not because they are somehow invalid, but not used by the project (like exporting ClassDeclaration) and I throw then too - those can be implemented when needed.

I dont feel like this transform needs any additional tests, if it mess up with the code regular tests should catch it, so in a sense they are testing this transform too, because they test transformed code.

[kedashoe]

This is great @Andarist , thanks!

I'd like to see a more minimal implementation to start out. What are your thoughts on:

• a file scripts/babel/esm-to-cjs.js
• let's get rid of the error checks for a minute
• rather than looping and searching in Program / exit, having visitors for ExportDefaultDeclaration and ImpotDeclaration (let's move these to top level functions as well, always bothers me all the indentation you end up with with babel)

eg

function exportDefaultDeclaration(path) {
  var declaration = path.get('declaration');

  if (declaration.isFunctionDeclaration()) {
    var id = declaration.node.id;
    if (id) {
      path.replaceWithMultiple([
        declaration.node,
        buildExportsAssignment(
          t.identifier(id.name)
        )
      ]);
    } else {
      path.replaceWith(
        buildExportsAssignment(declaration)
      );
    }
  } else {
    path.replaceWith(
      buildExportsAssignment(declaration.node)
    );
  }
}

function importDeclaration(path) {
  // etc
}

module.exports = function() {
  return {
    visitor: {
      ExportDefaultDeclaration: exportDefaultDeclaration,
      ImportDeclaration: importDeclaration,
    }
  };
};

Could you leave the current file, scripts/transform-es2015-modules-bare-cjs.js, around for now as well to make it easier to see what we've done.

[Andarist]

@kedashoe I've prepared a simpler transform here (its ofc part of the PR now, just pointing to it directly as navigating through this this 374 file PR is not handy 😄 )

I have left some (only like 3 now) mentioned checks in place, as I was not so sure that those were safe to remove - transform code make some assumptions about the structure of the project and is not a universal transform. We can ofc remove those too, ur call here :)

[kedashoe]

I've prepared a simpler transform here (its ofc part of the PR now, just pointing to it directly as navigating through this this 374 file PR is not handy 😄 )

Thanks looks good!

I have left some (only like 3 now) mentioned checks in place, as I was not so sure that those were safe to remove - transform code make some assumptions about the structure of the project and is not a universal transform.

Sounds good.

Can we add another transform file to automatically insert the PURE annotations? Annoying seeing them in the source code imho. Let me know your thoughts on that.

[Andarist]

Can we add another transform file to automatically insert the PURE annotations? Annoying seeing them in the source code imho. Let me know your thoughts on that.

Sure thing, although I would write my own for now. Gonna work with annotate-pure-call-in-variable-declarator's author simultaneously though so in the future we could drop custom transform and replace it with it, because at the moment we cannot use it as is.

[Andarist]

@kedashoe ok, so I've written a super simple plugin for annotating those top-level calls automatically and it does a great job for ramda's use case

[kedashoe]

@Andarist @jonaskello @olsonpm / whoever else would like to test it out, I've publish this WIP to npm

npm install ramda@es-rc

Any feedback greatly appreciated!

[olsonpm]

will do tomorrow. i have a perfect small ephemeral work project to test it out with.

glad to see you found the time for open source! once my new job winds down a bit i'm hoping to hop back in.

[Andarist]

@kedashoe

Followuping here to the #1968 (comment)

Several files (the ones using 'constructor pattern') needs to wrapped in IIFEs for better tree-shakeability. There are actually 3 ways we could approach this:

• wrapping in IIFEs by hand
• write a custom babel transform for wrapping
• use es6 classes and transpile them down to es5 with standard babel-plugin-transform-es2015-classes, this would produce smth like this

[kedashoe]

I'd say lets avoid classes for this PR. I like the babel transform since it makes it obvious why we are doing it. How will you determine when to trigger it? Detect prototype assignment?

[Andarist]

Yeah, prototype assignment + traversal upwards, gathering whole 'class' together and wrapping it in IIFE. Gonna cook this up when I have time :)

[olsonpm]

Well considering you guys went with the pure annotation approach, I'm less interested in this branch. Comparing the bundled output of ramda@es-rc vs mine (where I manually added the pure annotations), yours produces a lot of unused code - implying the annotation plugin being used needs some work.

[CrossEye]

After some serious prodding from @kedashoe, I've finally spent some real time with this and believe I understand pretty well what's going on. Sorry it's taken me so long.

🌿

I'd be very happy to see this move forward.

[Andarist]

I have just spotted that I probably need to restore old build script (probably tweaked or something). I haven't noticed that it was used also for partial builds.

[kedashoe]

💯 thank you for all your hard work on this @Andarist and everyone else who helped with this one!