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

AMD #6

Closed
fredck opened this issue Aug 27, 2014 · 22 comments
Closed

AMD #6

fredck opened this issue Aug 27, 2014 · 22 comments

Comments

@fredck
Copy link
Contributor

fredck commented Aug 27, 2014

A proposal for AMD in the CKEditor development code has been pushed to the amd prototype branch.

Read the README file there first, to have a general understanding about the goals of the prototype.

@fredck fredck added the review? label Aug 27, 2014
@fredck
Copy link
Contributor Author

fredck commented Aug 27, 2014

One important note about the proposed approach: no AMD will be available outside of our core code. This means that plugins, or anything that is to be loaded on demand, will not be coded on AMD.

The reasoning behind this is that we cannot require any AMD library to be able to run the build version of CKEditor. Therefore, on demand code should also not require AMD libraries.

There would be a solution for this. We could provide CKEditor specific AMD support in our API, namely CKEDITOR.define and CKEDITOR.require, which would be then used by plugins instead of define and require. To make it possible though we will have to include an AMD library (RequireJS) inside our distribution code, raising the size of ckeditor.js considerably (RequireJS is 16KB).

@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2014

I agree will all this. We definitely shouldn't require or ship with an AMD library.

However:

  • We must remember that the built version of CKEditor should be useable as an AMD module. Most likely this means that there must an additional file with contents of ckeditor.js wrapped with AMD closure (let's call it ckeditor-amd.js).
  • Development version of CKEditor should also be usable as an AMD module, so it is possible to easily switch between dev and prod environments.
  • This has to be synchronised with our discussion about the builder. This most likely means that /ckeditor.js file cannot be a part of the repository, because it will be created by builder (in dev and prod envs). Of course its content can be built based on ckeditor-base.js that will belong to the repository.
  • Builder may have an --amd flag which when used will create AMD-ready ckeditor.js. This applies to dev and prod envs (though in prod env the result will be saved to e.g. ckeditor-amd.js).

@gregpabian
Copy link

Hmm.. why not UMD?
There are some nice patterns shown, besides there's already a grunt plugin for it https://github.com/bebraw/grunt-umd

@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2014

I don't know UMD, so I'm not commenting it now. But a general advice for us should be that only well defined, and widely used libraries/concepts should be adopted. Architecture that we're now designing will be used by us for next 10+ years, so last season's solutions are a risk.

@Reinmar
Copy link
Member

Reinmar commented Aug 28, 2014

We continued with @fredck a discussion about a case when developer wants to load plugins on demand (possibly from locations outside CKEditor directory).

Currently CKEditor's core handles loading plugins. External plugins can be registered using addExternal() and then CKEditor knows where to load them from.

Since we are thinking about using AMD for the core of v5 we could also consider using AMD in case of plugins. However, that would mean that in prod env RequireJS or other library is a new requirement. This is 16kb that would never be used by developers that have precisely configured builds, so we should avoid it. Additionally, a project may already use RequireJS and then code would be duplicated.

Possible solutions:

  1. CKEditor should avoid doing magic. Currently it does all by itself - it's really a cool mechanism, but it's confusing, especially for newcomers. So what if it stops loading plugin files by itself? If e.g. you want to use jQ plugin you must include it manually using a script tag or any mechanism which your project use. We could expect the same - you want to load additional plugin, then load it. CKEditor will only initialise it.

    Unfortunately, this solution has a downside - CKEditor must know plugin's root path so it can load an icon, CSS or any other asset. So not only a developer would need to add a script tag but also tell CKEditor what's the path to the plugin. It's very similar to current situation, so it's not that bad.

  2. Since a developer has to specify the path anyway, perhaps CKEditor could still load the plugin. Merging the solution 1. with what I wrote about unwelcomed Require.JS requirement I came up with another idea. What if we don't merge RequireJS into CKEditor, but ask developer to include it if (s)he wants to load plugins on demand?

    In this case we would need to assure one thing - that all plugins, including these merged into the ckeditor.js, are registered as AMD modules, so RequireJS is able to solve dependencies. However, CKEditor would also need to be fed with information about dependencies, because it affects also plugins initialisation order (not only files loading order).

    Since RequireJS is available conditionally we of course need a facade for it (namely CKEDITOR.define()). The facade will use RequireJS if it's available or our, simple replacement which will have ability to handle only plugin definitions merged into ckeditor.js.

    I've been playing with RequireJS and it seems that we can use it for plugins in this way:

    // Contents of ckeditor.js:
    define( 'ckeditor/plugins/foo', function() {
      return 1;
    } );
    define( 'ckeditor/plugins/bar', function() {
      return 2;
    } );
    // ckeditor.js loads plugins:
    require( [ 'ckeditor/plugins/foo', 'ckeditor/plugins/bar', 'ckeditor/plugins/bom' ], function( foo, bar, bom ) {
    
    } );
    
    // Contents of my-plugins/bom.js:
    define( 'ckeditor/plugins/bom', function() {
      var pathToPlugin = requirejs.toUrl( 'ckeditor/plugins/bom' );
    
      return 1;
    } );
    
    
    // Developer has the possiblity to configure the location of any plugin:
    requirejs.config( {
      paths: {
        'ckeditor/plugins/bom': 'my-plugins/bom'
      }
    } );
    

    Note: CKEDITOR.define will need to enforce that /plugin or /<name> is appended to every module name so we keep plugins in separate directories. On the other hand, if it didn't do that we could have a wayt to load multiple JS files from plugin's directory. Not bad too.

    The remaining questions:

    • Will CKEditor know in which order plugins should be initialised? There are two options - since the CKEDITOR.define() will be used with plugin's requirements CKEditor may remember that and then solve dependencies. Or each plugin (==module) will return a structure pointing to its requirements (already resolved by RequireJS), so CKEditor will only traverse this graph.
    • Are we able to enforce that if CKEditor itself is loaded as an AMD module it's loaded before its plugins, so CKEDITOR.define() is ready? I think that yes, because additional plugins will be loaded when editor creator is executed, so inside require( 'ckeditor', function( CKEDITOR ) { ... } );.
    • What will happen if someone tries to use RequireJS's optimiser? A catastrophe, because it won't handle plugins' assets. Unless the paths specified in requirejs.config() are used by requirejs.toUrl().

Sadly, it seems that if we want to make use of RequireJS and be compatible with it we must be very careful or there will be many constraints. This raises a question whether it's worth trying. Whether the first solutions isn't better. In any case, if a developer will want a highly optimised page on which CKEditor is compiled with other libraries, (s)he can use CKEditor's builder to create a single-file package. We should just make sure that there's also a possibility to build a ckeditor.js file which is a correct AMD module.

My KISS sense tells me that solution 1 with no code for dynamic JS loading at all (so no config.js and styles.js - want to extract them? include them manually - no more magic) and a clever builder as the one described in our email thread will do best (so the requirements are kept outside plugins in separate file and builder is used to create a loader file which is used in dev env to load all this). I'm even thinking whether stylesheets should be loaded automatically, because... why? It always make problems when you want to concatenate your assets.

@pjasiun
Copy link

pjasiun commented Sep 1, 2014

I'd like to add that I like the idea of using AMD (also) because it is a kind of dependency injection, and DI is great to make tests far more separated.

@Reinmar
Copy link
Member

Reinmar commented Sep 1, 2014

True. AMD will allow us to control DI and we planed to solve this issue anyway. So AMD used on the entire code base is not only for the beauty.

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2014

It struck me on yesterday's CopenhagenJS - could we use ES6 modules? We will need to use some converter for them, because they are not yet widely supported, but I'm sure there are some.

The good thing about ES6 modules is that one day we won't need any additional loader and for guys that work on limited environments (for instance Chrome/FF only) this day will come pretty soon.

There are of course many details why ES6 modules might not work for us, but I think that we should at least make a research about them.

@fredck
Copy link
Contributor Author

fredck commented Sep 4, 2014

Using ES6 modules would require us to compile our code to be able to run it. Especially because there is still no support for it. I'm not sure it is worth the effort and still the whole modules thing is under work, so we can expect changes on it in the future. It would be a very experimental way to go. Additionally, we would require people to understand things that they still don't understand. Our code would be really made for us only.

For instance, as compilation would be needed, we could even think about going ahead with many of the ES6 features by using tools like Traceur. Still we're entering in a field of fun... and pain. Additionally, we would have totally different ways of programming: one for core and another for external stuff (plugins).

AMD is already a standard and well accepted pattern. It's proving to be a good way to solve our problem. Hopefully we found a nice way to use it, so there is no additional benefit on complicating the present with ES6 modules aiming for a brighter future.

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2014

After a bit of thinking, I don't like ES6 modules. I was thinking only about our code, but there will be 3rd party plugins and we can't expect from their developers too much. AMD is better in this case.

As for the rest of ES6 features, AFAICR some of them are already stable in terms of specification. But I agree with you that by taking too much of it we would make our code harder to read for developers that don't follow standards so closely. On the other hand, there are some very tempting parts like block scoping, desctructuring assignments or classes which are simple to understand (unlike generators for which is too early IMO) but affects the way how you code significantly. In fact, if we started using ES6 classes we would be sure that they are always consistently created and used - veeery tempting. And we would make sure that our code looks fresh for a long time - we need to remember v5's life time.

The bottom line is, however, what's the performance of Traceur, how stable it is and what's the performance and size of code it generates. I'm afraid that if we start with it and will be dissapointed it will be very quickly hard to step back.

@Reinmar
Copy link
Member

Reinmar commented Sep 4, 2014

Another reason for not going ES6 way - documentation. We don't have a tool that's able to comprehend ES6 code.

Another reason - linting.

Another reason - code style checking.

So... nope :D.

@Reinmar
Copy link
Member

Reinmar commented Sep 12, 2014

BTW. Final syntax of ES6 modules has been defined - http://www.2ality.com/2014/09/es6-modules-final.html

@Reinmar
Copy link
Member

Reinmar commented Sep 22, 2014

Didn't fully read yet, but it touches a topic which worried me as well - http://orizens.com/wp/topics/a-journey-from-require-js-to-browserify/

@fredck
Copy link
Contributor Author

fredck commented Sep 24, 2014

Just one of the issues pointed by the article may eventually touch us, which is the potential size of the require list on the module definition. That is generally considered one of the AMD weaknesses, but this is just theory, because if you really have those dependencies you will have lots of require() calls inside your code anyway, in the CommonJS style. So it may look ugly, but it's not anything more than that.

I'm sure that all options have weak points, so we have to pick one. I would stick with AMD, RequireJS and the way proposed in the "amd" prototype branch.

@fredck
Copy link
Contributor Author

fredck commented Nov 24, 2014

Our approach to AMD has been described at the following wiki page: AMD

I would ask @gregpabian, @Reinmar and others to please give it a quick look and see if there is anything wrong or missing.

@gregpabian
Copy link

Root -> ckeditor-core/src/
E.g. 'tools/utils' points to ckeditor-core/src/tools/utils.js

plugin!<plugin_name> -> ckeditor-plugin-<plugin_name>/src/
E.g. 'plugin!image' points to ckeditor-plugin-image/src/image.js
E.g. 'plugin!image/something' points to ckeditor-plugin-image/src/something.js

When we talk about the development environment, the actual paths are:

  • root: node_modules/ckeditor-core/src/
  • plugins!<plugin_name> -> node_modules/ckeditor-plugin-<plugin_name>/src/

Besides, it's plugins!<name> not plugin!<name>.

The 'ckeditor' module

Not yet true for the prototype, but might be :)

@fredck
Copy link
Contributor Author

fredck commented Nov 24, 2014

When we talk about the development environment, the actual paths are:

root: node_modules/ckeditor-core/src/
plugins!<plugin_name> -> node_modules/ckeditor-plugin-<plugin_name>/src/
Besides, it's plugins! not plugin!.

Ok, fixed.

The 'ckeditor' module
Not yet true for the prototype, but might be :)

Yes, the documentation must make it clear how it will look like, not simply describe the prototype. Based on the docs we gonna start the real implementation.

@Reinmar
Copy link
Member

Reinmar commented Nov 24, 2014

I would ask @gregpabian, @Reinmar and others to please give it a quick look and see if there is anything wrong or missing.

I actually haven't yet check AMD at all and I didn't have time to reply to Grzesiek regarding MVC. I'll do that tomorrow.

@fredck
Copy link
Contributor Author

fredck commented Nov 25, 2014

Note that I'm asking for review on the AMD documentation, not on the prototype code. That code may be used just to understand better what the documentation is saying.

@theodoreb
Copy link

Just a quick comment about ES6: https://medium.com/@brianleroux/es6-modules-amd-and-commonjs-c1acefbe6fc0
As for linter and syntax checking, eslint is on it: http://eslint.org/blog/2014/11/es6-jsx-support/

Just for the record, I like AMD too :)

@fredck
Copy link
Contributor Author

fredck commented Dec 10, 2014

Just a quick comment about ES6: https://medium.com/@brianleroux/es6-modules-amd-and-commonjs-c1acefbe6fc0

Yes, the future will be wonderful! We took this in consideration, but to be able to use it (partially) we would have to have code pre-processors and we want to avoid this. So, for now we're going with RequireJS.

@fredck
Copy link
Contributor Author

fredck commented Feb 18, 2015

@fredck fredck closed this as completed Feb 18, 2015
@fredck fredck mentioned this issue Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants