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

ES6 Modules #73

Closed
fredck opened this issue Oct 8, 2015 · 25 comments
Closed

ES6 Modules #73

fredck opened this issue Oct 8, 2015 · 25 comments
Labels

Comments

@fredck
Copy link
Contributor

fredck commented Oct 8, 2015

People are questioning the reason for not adopting ES6 Modules in v5. I'm opening this issue to have this discussed, decided and clarified.

@fredck
Copy link
Contributor Author

fredck commented Oct 8, 2015

The reasons to have ES6 Modules today:

  • Future-proof: in 5 years from now, everybody will be coding with it (hopefully!).
  • It would make us look “cool” today.

Other than the above, I don’t see any other benefit on ES6 Modules over AMD (modules scope and other architectural differences are really minor benefits).

We have quickly discussed about ES6 Modules earlier in #6 (comment). The main factors for us to adopt AMD was:

  • Chrome doesn’t support ES6 Modules natively. No browser supports it, actually.
  • Avoid compilation: we didn’t want to include the compilation process in our workflow. Additionally, compilation would be yet another complication for plugin developers.

Other minor issues where taken in consideration as well. Still, let’s remember that that talk happened over a year ago… and, surprisingly, little changed on the status of ES6 Modules out there since then, except that there is more buzz around it.

Apart from the above issues let me add other issues about moving from AMD (and RequireJS) to ES6 Modules:

  • ES6 Modules have to point to js files. Considering the way we have the project split on different repositories and the fact that we’re plugin based, it would be extremely complicated to make references cross repos.
  • We have an “isolated” namespace for modules loading. Root points to ckeditor5-core. So we do, for example, require( 'tools' ), not require( '../../../some/path/tools' ).
  • About modules names, we have RequireJS plugins, which allow us to easily make references to plugins with require( 'plugin!my-plugin' ).
  • We use the power of r.js for building. Anyway, I should assume that there may be a similar solution for ES6 Modules.
  • We have an isolated AMD system in our build, with CKEDITOR.define and CKEDITOR.require. It is a guarantee of zero-collision with any module system available in the page.
  • It is still unclear how building would work when merging several ES6 Module based files.

Please correct me if I’m wrong, because I’m far from being a ES6 Modules expert.

Additionally, a few facts:

  • We can certainly make CKEditor distributions loadable with ES6 Modules syntax. So we’re not talking about a problem on distribution, but an internal development strategy.
  • We can, at any point in the future, move the code to ES6 Modules, if it will ever make sense. Major releases are here for that and maybe we’ll be able to handle backwards compatibility somehow.

Based on all the above, as for today, I’m for AMD.

@Reinmar
Copy link
Member

Reinmar commented Oct 12, 2015

👍 for all the arguments. Especially for the facts that:

  • we can always move to ES6 Modules (if we'll find a way to translate our plugins architecture to them), even just before 1.0,
  • without native support we will only complicate things for us.

@Reinmar
Copy link
Member

Reinmar commented Oct 12, 2015

BTW. For the future reference – System (the default loader) implements couple of methods that could enable us some tweaks like the current one. Also, there's Reflect.Loader constructor which is subclassable so perhaps even more can be achieved with it. However, this is still WIP, so it's too early to think about this.

@ipeychev
Copy link

Well, if you don't want to transpile the files, then there is nothing to do. Otherwise, a common approach is to write the code in ES2015 and to transpile then - using Babel for example. You can transpile the modules to AMD without any issues and to use r.js or whatever you want after that.
For me it is weird decision in 2015 to not transpile... the argument was this will be a complication for plugin developers. What complication exactly? They will still get AMD modules for now, until browsers start to support ES6 modules. And when they do, you will just stop transpiling from ES6 modules to AMD.
Browsers will be if not always at least in the nearest future behind the spec and the proposals, so if you wait until all of them get ready, your code will be behind for years too.

Just my two cents, hope that helps!

@fredck
Copy link
Contributor Author

fredck commented Oct 13, 2015

I understand that it is a current trend to go with transpilers and everybody is anxiously jumping into it. But should we take everything form ES2015 just because of that? We're already smartly adopting some very cool things from it, like promises and classes.

AMD is great and even preferred over ES6 Modules by many. So we're far from making a mistake if we decide to go with it.

Another problem is the loader, in fact. This is a completely "under works" part of the specs. I think Babel not even supports it. Also, the fact that no browser support modules still is a sign that this topic is not fully defined.

One key aspect of our current AMD implementation is that it doesn't depend on RequireJS on the page. We create a "secure" (isolated) AMD environment that avoids module name collision with third-party code. That's why we have CKEDITOR.require() and CKEDITOR.define(), which are simply pointers to Almond (build together with CKEditor). Because of this we can have modules references like the following example:

CKEDITOR.define( 'plugin!myplugin', [ 'ckeditor', 'tools', 'plugin', 'ui!button' ], function( CKE, tools, Plugin, Button )

No matter where this plugin is created, if it included in a page with CKEditor it will always work. It would never conflict with third-party code that provides a "tools" module.

I would be curious to understand what this would look like with ES6 Modules. (this is not a sarcastic statement... I'm really curious)

Btw, a note on transpilers... while we don't need them for daily development of CKEditor 5 (we target Chrome for development), we will still need them when producing release builds. The fact is that even the few things we're deciding to adopt form ES2015 right now are not available in all browser. All summed, I don't feel that we're doing something so critically wrong... it sounds like pretty aligned to 2015 :)

@Reinmar Reinmar added the es6+ label Oct 13, 2015
@ipeychev
Copy link

fredck: I understand that it is a current trend to go with transpilers and everybody is anxiously jumping into it. But should we take everything form ES2015 just because of that? We're already smartly adopting some very cool things from it, like promises and classes.

That is good, despite IMO classes in ES6 are still lean and mean. For example, no class annotations, no class properties, etc. ECMAScript will eventually go where TypeScript is now...
Anyway, in my previous company we adopted the promises and classes too, but we also adopted more things from ES2015. Babel handles these.

fredck: AMD is great and even preferred over ES6 Modules by many. So we're far from making a mistake if we decide to go with it.

With all my respect, I personally don't share this opinion. In fact, I'm pretty sure AMD spec was invented by the Devil, or at least he has a finger in the pie. Otherwise, it is hard to explain the complete mess, which these require and define functions are. I'm pretty sure a regular developer, who uses AMD, can't tell what kind of parameter combinations define supports on top of his head. Not to mention the idea of module and exports as dependencies... Both define and require are for the hall of shame.

fredck: Another problem is the loader, in fact. This is a completely "under works" part of the specs. I think Babel not even supports it. Also, the fact that no browser support modules still is a sign that this topic is not fully defined.

Umm, I'm sorry, but Babel has nothing to do with any loader. Babel is able to translate ES6 modules to CommonJS or AMD (and to System, UMD and some custom format, but this is without importance now). The whole story is:

  • you write your modules as ES6
  • then you transpile the files with Babel, and it will translate them to AMD
  • you run r.js or you use Almond or whatever to load these.

With the loader is completely "under works" part of the spec, you probably mean this spec, the programmatic loader API, correct? If so - yes, it is WIP, but no one says you have to use it. I was talking about the declarative syntax (importing and exporting).

fredck: One key aspect of our current AMD implementation is that it doesn't depend on RequireJS on the page. We create a "secure" (isolated) AMD environment that avoids module name collision with third-party code. That's why we have CKEDITOR.require() and CKEDITOR.define(), which are simply pointers to Almond (build together with CKEditor). Because of this we can have modules references like the following example:

CKEDITOR.define( 'plugin!myplugin', [ 'ckeditor', 'tools', 'plugin', 'ui!button' ], function( CKE, tools, Plugin, Button )
No matter where this plugin is created, if it included in a page with CKEditor it will always work. It would never conflict with third-party code that provides a "tools" module.

I would be curious to understand what this would look like with ES6 Modules. (this is not a sarcastic statement... I'm really curious)

This is cool, and it is how it should be, the namespace shouldn't be populated with functions like require and define.

I'm not sure if I will be able to explain in short how your code will look like if you adopt ES6 modules, but imagine in some file which consumes (uses) modules you will have:

import Core from 'src/core';
import Button from 'src/ui/button';
import {Dialog} from 'src/ui/dialog';
....

and let's say Button will be defined in src/ui/button.js like this:

import Widget from 'src/ui/widget';

class Button extends Widget {
   constructor() {
     super();
   }
}

export default Button;

fredck: Btw, a note on transpilers... while we don't need them for daily development of CKEditor 5 (we target Chrome for development), we will still need them when producing release builds. The fact is that even the few things we're deciding to adopt form ES2015 right now are not available in all browser. All summed, I don't feel that we're doing something so critically wrong... it sounds like pretty aligned to 2015 :)

Again - I think using a transpiler is a huge advantage. The build system of CKEditor was Java based, right? And I guess you are using it only when you generate the dist files? I would drop this and switch to Node, put Babel in the middle and be happy. But it is up to you.

Hope that helps and sorry if some parts of what I wrote sound rude. This has nothing to do with your work on CKEditor, I respect it very much.

Thanks,

@fredck
Copy link
Contributor Author

fredck commented Oct 13, 2015

Thanks for the details.

IMHO, AMD, as anything else, if used properly is extremely powerful. We use the good parts of it and so far the experience with it is wonderful.

Nothing agains ES6 Modules though, as long as we're able to have the same features we have with AMD.

and let's say Button will be defined in src/ui/button.js like this:

And here we have the problem and why the loader API is essential for us.

The "Button" plugin, has zero idea about "src/". It is coded in a totally different repository which is loaded together with the CKEditor 5 API core using NPM. It wants the "ui/widget" class from core, but it doesn't know where to find it.

We could eventually make it work by hooking into transpiling, converting raw requests (like "ui/widget") to their full paths. But then the code will depend on transpilers forever. It'll not be ready for the future when we'll finally through transpilers away.

We could "assume" that one day a API for loaders will be available and so we could use it to provide a similar module name expansion system. Still, forgetting about placing in a third-party page a file with import Widget from 'ui/widget'. You'll be asking for naming collision issues.

Therefore the only imaginable way for this is prefixing all module names with a "unique" string. Something like ckeditor!ui/widget and ckeditor-plugin!button. This would make us change the current approach:

CKEDITOR.define( [
    'model',
    'editorconfig',
    'plugincollection',
    'promise'
], function( Model, EditorConfig, PluginCollection, Promise ) { ... } );

... to:

import Model from 'ckeditor!model';
import EditorConfig from 'ckeditor!editorconfig';
import PluginCollection from 'ckeditor!plugincollection';
import Promise from 'ckeditor!promise';

Now, questions to all of us:

  1. Do we want to have such module naming strategy?
  2. Do we want to use transpilers on development?
  3. Do we want to force plugin developers to have more complex development environments? (that's because they'll be also forced to compile the whole editor with their plugins, which currently is not necessary)
  4. About the above "3", are we ok to drop the possibility of adding plugins to the page by simply using <script> elements? (plugins will have to be always part of builds)
  5. Are we ok to move ahead with the hope that a loader API will be implemented into browsers some day? (loaders are not part of ES2015 still, unlike modules. Still the missing loader API seems to be the reason why browsers still didn't move about implementing ES6 Modules at all)

@ipeychev
Copy link

fredck: 1. Do we want to have such module naming strategy?

One solution is to write:

import Model from 'ckeditor/model';

then Babel will transpile this to:

define(['exports', 'ckeditor/model'], function (exports, _ckeditorModel) {

In any AMD loader you can then specify what exactly ckeditor is (via paths, map, etc.).

You can use any syntax, for example:

import Model from 'ckeditor:model';

to point users ckeditor is a special path, path so some other repo or whatever.

fredck: 2. Do we want to use transpilers on development?

I guess you don't. I personally would love to, I think they make sense.

fredck: 3. Do we want to force plugin developers to have more complex development environments? (that's because they'll be also forced to compile the whole editor with their plugins, which currently is not necessary)

No, this one we don't want. I don't quite understand why they will be forced to compile the whole editor, maybe I'm missing something. I've written a few CKEditor plugins and I remember the API was: CKEditor.plugin.add('name-of-the-plugin', {...});
This you may continue to have. Maybe you spot some other issue here, I'm not an expert here.

fredck: 4. About the above "3", are we ok to drop the possibility of adding plugins to the page by simply using script elements? (plugins will have to be always part of builds)

No, this would be something bad.

fredck: 5. Are we ok to move ahead with the hope that a loader API will be implemented into browsers some day? (loaders are not part of ES2015 still, unlike modules. Still the missing loader API seems to be the reason why browsers still didn't move about implementing ES6 Modules at all)

Well, this is a tough question. Since you are starting a new version and great features will be added, I thought it would be useful to research for the possibility to move to ES2015 modules instead to continue to use something which definitely does not have future (AMD).

Hope that helps!

Thanks,

@glen-84
Copy link

glen-84 commented Oct 30, 2015

@fredck How do you plan to use classes without transpilation?

I'm not sure why you're trying to avoid transpilation, it's quite easy to set up (with gulp and babel for example), and you get all the benefits of using ES2015 now.

You should even consider TypeScript, for easier refactoring, auto-completion, etc.

This doesn't mean that plug-in authors will have to use the same tools, they would be free to use ES5 if they wanted to.

JSPM is also something to look at.

@fredck
Copy link
Contributor Author

fredck commented Nov 5, 2015

@fredck How do you plan to use classes without transpilation?

As of today, we can use classes in Chrome without transpilation. That's why we're ok with them.

Anyway, we've been discussing about the ability of making CKEditor usable inside Node, so a building process may end up as part of our workflow, which would enable transpilation.

@Reinmar
Copy link
Member

Reinmar commented Nov 5, 2015

If we target browsers and Node.JS at the same time, I think it makes ES6 modules most reasonable choice. The reason is that this module system is not affected by any of the two environments and it will require us to built for both of them. This clarifies the situation, because writing for browser and building for Node (or vice versa) means that you have some env-specific stuff in the first place and you try to translate it for the other env. By choosing ES6 modules we would avoid that. We start with a clean architecture and make the builder smart (or simply choose Webpack or something :D).

@fredck
Copy link
Contributor Author

fredck commented Nov 5, 2015

If we go for requiring building, I'm all for ES6 Modules, as long as we clearly figure out how to handle the issues I've commented before.

Anyway, as for today, this would still be a shot on the dark, because there are still no standards for the loader. Risking would be fine right now, being aware that in the future we may have to review some things once the standard comes out.

@glen-84
Copy link

glen-84 commented Nov 7, 2015

As of today, we can use classes in Chrome without transpilation. That's why we're ok with them.

i.e. You were hoping that by the time CK5 ships, all the major browsers would support classes? ... or you only plan to support Chrome, LOL.

There are so many other cool things in ES2015 and even ES2016 (class properties, class decorators, etc.) that it would be/would have been a shame not to make use of that.

As for module loading, from what I understand (which is very little), you can transpile ES2015 modules to AMD/CJS/SystemJS/UMD and then use a loader for one of those formats.

@Reinmar
Copy link
Member

Reinmar commented Nov 9, 2015

As of today, we can use classes in Chrome without transpilation. That's why we're ok with them.

i.e. You were hoping that by the time CK5 ships, all the major browsers would support classes? ... or you only plan to support Chrome, LOL.

Please read https://github.com/ckeditor/ckeditor5-design/wiki/Coding#ecmascript-6-2015-2016 before making such comments.

@glen-84
Copy link

glen-84 commented Nov 9, 2015

@Reinmar I was just joking about the Chrome-only support (hence the "LOL").

@Reinmar
Copy link
Member

Reinmar commented Nov 9, 2015

Sorry, it sounded like mockery ;). Maybe because some devs really have such an approach.

@pjasiun
Copy link

pjasiun commented Nov 10, 2015

There are 2 things in the Require.js and our current implementation I really do not like: no solving circular dependencies and asynchronous loading.

For example for asynchronous loading, I wanted to create a class inside a test, for a testing reason, a child class. I was not able to create it at the begging of the testing file, because the base class was not loaded yet. I had to do it in the before function. But then I was not able to declare it in the standard way, because the class was not available outside that function so I had to do:

var modules = bender.amd.require( 'base' );

describe( 'Bar', () => {
    var Test;

    before( () => {
        Base = modules.base;

        Test = class extends Base {
            constructor( f ) {
                super( f );
                // ...
            }
        };
    } );

    it( 'should test something', () => {
        // ...
    } );
} );

I would love to write:

import Base from "base";

class Test extends Base {
    constructor( f ) {
        super( f );
        // ...
    }
};

describe( 'Bar', () => {    
    it( 'should test something', () => {
        // ...
    } );
} );

@fredck
Copy link
Contributor Author

fredck commented Nov 10, 2015

The asynchronous issue on tests you talked about has nothing to do with this topic. We're talking about modules in the CKEditor code, while you're talking about modules in the test environment. Two different things and I assume different solutions would be found for this.

What I can tell for sure is that bender.amd.require() has not been introduced to load modules that are out of the CKEditor API range.

@Reinmar
Copy link
Member

Reinmar commented Nov 12, 2015

Super informative presentation about ES6 modules – http://benjamn.github.io/empirenode-2015. It clarifies how ES6 modules would solve the circular deps problem that we're dealing with all the time. It also confirms what I wrote before that having a syntax for modules rather than an API allows for creating greater features and for static analysis.

I'm going to research this topic further now. I'm super curious what I'm going to find :)

@ipeychev
Copy link

If you have circular dependencies, you simply should fix your code and not rely on ES6 to solve the problem in some magic way :)

Besides that, I am happy that you finally realized the potential of ES6 modules and started to consider dropping AMD.
👍

@Reinmar
Copy link
Member

Reinmar commented Dec 10, 2015

And while we're working on porting our code to "ES6 modules" you can find this: https://twitter.com/medikoo/status/674947685946904576

@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2016

We fully adopted ES6 modules. It was a long process as we had no idea what will be the most flexible and safe way of adopting them in our case and there were many problems along the way, but the final result works fine with just a one hack – a patched Require.JS solving https://phabricator.babeljs.io/T6880?workflow=create#R – it's very clean.

I updated related topics in the wiki:

Feel free to add your 2cents. There's still a lot we could learn about workflows with ES6 modules.

@ipeychev
Copy link

ipeychev commented Feb 3, 2016

Great, +1 from me for this decision!

@glen-84
Copy link

glen-84 commented Feb 7, 2016

Awesome. Now you just need to switch to TypeScript. 😁

@Reinmar
Copy link
Member

Reinmar commented Feb 7, 2016

Yeah, right after we switch to Dart :P.

@Reinmar Reinmar closed this as completed Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants