Ckeditor5: Default language (en) is not loading with RequireJS (AMD)

Created on 18 Mar 2018 · 33Comments · Source: ckeditor/ckeditor5

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

CKEditor 5 build v1.0.0-beta.1

📋 Steps to reproduce

Example Code

<html>
<head>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.5/require.min.js"></script>
    <script>
        require(['https://cdn.ckeditor.com/ckeditor5/1.0.0-beta.1/balloon/ckeditor.js'], function(cke5) {
            cke5.create(document.getElementById("edit"));
        });
    </script>
</head>
<body>
    <div id="edit">
        <h1>For Test</h1>
    </div>
</body>
</html>

❎ Actual result

Uncaught ReferenceError: CKEDITOR_TRANSLATIONS is not defined at ckeditor.js:7

ckeditor5-lang-error

bug

Source

nao-pon

Most helpful comment

I've pushed changes to the t/914 branches and I'm writing here a description what is done so far.

CKEditor5WebpackPlugin

RawSource is used instead of normal objects for generating assets because there was a problem with ConcatSource used internally by Webpack.BannerPlugin
Name of the event being listening to is changed (from emit to optimize-chunk-assets) to emit assets before Webpack.BannerPlugin, so the license is added at the correct place (not after translations).

MultipleLanguageTranslationService (ckeditor5-dev-utils)

Changed body of generated chunks and translations. Now there's something like this at the beginning because RequireJS can load these translations in a different order:

'(function(d){' +
    `d['${ language }']=Object.assign(` +
        `d['${ language }']||{},` +
        `${ stringifiedTranslations }` +
    ')' +
'})(window.CKEDITOR_TRANSLATIONS||(window.CKEDITOR_TRANSLATIONS={}));'

It slightly increases the size of assets, but allows to use both approaches:

<script src="translations/pl.js">
<script>requirejs( [ 'ckeditor.js' ], ClassicEditor => {} )</script>

<script>requirejs( [ 'ckeditor.js', 'translations/pl.js' ], ClassicEditor => {} )</script>

TranslationService (ckeditor5-utils)

Removed add() function (which was adding translations).
Instead, this service uses now window.CKEDITOR_TRANSLATIONS to get all translations when they're needed (when translate() function is invoked).

Builds and samples:

We can improve our samples and add UMD sample and translation sample. (These samples will only need HTML files and it's not possible to create them using manual tests.

ma2ciek on 20 Mar 2018

👍2

All 33 comments

@ma2ciek could you check it?

Reinmar on 19 Mar 2018

Yes, it's a bug. When loading editor's build through RequireJS, the exported function evaluates after the translations added at the end of that script. Maybe to ensure the script's asynchrony or lazy loading, I don't know.

The Webpack's UMD looks as follows:

(function(e, t) {
    'object' == typeof exports && 'object' == typeof module ? module.exports = t() : 'function' == typeof define && define.amd ? define([], t) : 'object' == typeof exports ? exports.BalloonEditor = t() : e.BalloonEditor = t()
}
)('undefined' == typeof self ? this : self, function() {
   // Script
} );

So for all options despite AMD, the function with script's body runs synchronously and translations should be added correctly. I didn't know that behavior.

ma2ciek on 19 Mar 2018

Do you have an idea for how to workaround this?

Reinmar on 19 Mar 2018

I'm thinking now about some smart reversing, so the translation will be added globally before the editor. I'll make some quick POC.

ma2ciek on 19 Mar 2018

We could have something like this:

cke-build.js

var CKEDITOR_TRANSLATIONS = { 'en': { ... } };
// Script

pl.js

CKEDITOR_TRANSLATIONS[ 'pl' ] = { ... }

And translation-service will use CKEDITOR_TRANSLATIONS or copy of that as a dictionary.

But now I'm unsure about running many editors. If it will affect previous ones as require.js is asynchronous in loading them.

ma2ciek on 19 Mar 2018

TBH, you'd need to remind me how it works now :D Which module exports what and how it's executed by RequireJS.

Reinmar on 19 Mar 2018

Sure ;)

Now the workflow is as following:

translation-service is a service that provides translations for all t() calls and exports add function globally, so it can be used later: window.CKEDITOR_TRANSLATIONS.add( 'en', {} )

The English translations are added to the build at the end, so the build for the balloon editor looks like this:

(function(e, t) {
    'object' == typeof exports && 'object' == typeof module ? module.exports = t() : 'function' == typeof define && define.amd ? define([], t) : 'object' == typeof exports ? exports.BalloonEditor = t() : e.BalloonEditor = t()
}
)('undefined' == typeof self ? this : self, function() {
   // Whole code of the CKEditor5 with exported BalloonEditor.


} );

CKEDITOR_TRANSLATIONS.add( 'en', {} );

The RequireJS's define function is called in the second line: define([], t), where the t is the second parameter of the function and it becomes a function with the whole CKEditor's logic. I don't know how RequireJS invokes that t function later, but as we can see it runs it async, so the CKEDITOR_TRANSLATIONS.add( 'en', {} ); is called before t and the error is thrown.

ma2ciek on 19 Mar 2018

OK, so the module's wrapping function is executed on demand – when first requiring (aka importing) anything from such a module.

Perhaps, the module added by CKEditor5WebpackPlugin could require the translation-service module first to force its evaluation? This may cause some ugly circular dependency which RequireJS might not be able to solve, but it's worth checking.

Reinmar on 19 Mar 2018

But then we'd have to know what's the name of the current AMD module and I'm not sure if that information is available.

ma2ciek on 19 Mar 2018

Current AMD modules? What do you mean by "current" and why we'd need to know that?

To be clear – I meant that perhaps CKEditor5WebpackPlugin could add something like this:

// Import whatever from there to force its evalutation.
import add from '@ckeditor/ckeditor5-utils/src/translation-service';

CKEDITOR_TRANSLATIONS.add( 'en', {} );

Is it possible?

Reinmar on 19 Mar 2018

Actually, I guess – not. And the reason would be our file split (than lang files are separated), yes?

Reinmar on 19 Mar 2018

I misunderstood your last question. That's not possible due to the fact that all webpack imports with our stuff are resolved and land in the 4th. line - see my snippet above.

ma2ciek on 19 Mar 2018

Webpack provides an UMD wrapper, so the code resolved from all imports is actually wrapped in the function, which is later called depending on the runtime - define( fn, [] ) for require.js, module.exports.BalloonEditor = fn() for common.js or window.BalloonEditor = fn() for manually added script.

ma2ciek on 19 Mar 2018

https://github.com/ckeditor/ckeditor5/issues/914#issuecomment-374175258 - I've tested that approach and it works, because the CKEDITOR_TRANSLATIONS is declared before the async code execution.

ma2ciek on 19 Mar 2018

Yeah, I came to the same conclusion that we can't use add() and that we should manually push stuff to CKEDITOR_TRANSLATIONS. However, I'd use some safe solution like:

CKEDITOR_TRANSLATIONS.pl = Object.assign( CKEDITOR_TRANSLATIONS.pl || {}, ...translations );

Reinmar on 19 Mar 2018

IMO, we may want to add translations for each language only once. I don't see a use case for the safer version.

ma2ciek on 19 Mar 2018

People adding their translations from outside of a build?

BTW, what I proposed is exactly what add() does now:

export function add( lang, translations ) {
    dictionaries[ lang ] = dictionaries[ lang ] || {};

    Object.assign( dictionaries[ lang ], translations );
}

And with the whole CKEDITOR_TRANSLATIONS object we're safe now too:

window.CKEDITOR_TRANSLATIONS = window.CKEDITOR_TRANSLATIONS || {};

I'd keep that.

Reinmar on 19 Mar 2018

People adding their translations from outside of a build?

Right, so I'll keep the safer version.

I'll prepare a PR with the fix.

ma2ciek on 19 Mar 2018

I'll add require.js manual tests to builds too within that PR.

ma2ciek on 19 Mar 2018

I've pushed changes to the t/914 branches and I'm writing here a description what is done so far.

CKEditor5WebpackPlugin

RawSource is used instead of normal objects for generating assets because there was a problem with ConcatSource used internally by Webpack.BannerPlugin
Name of the event being listening to is changed (from emit to optimize-chunk-assets) to emit assets before Webpack.BannerPlugin, so the license is added at the correct place (not after translations).

MultipleLanguageTranslationService (ckeditor5-dev-utils)

Changed body of generated chunks and translations. Now there's something like this at the beginning because RequireJS can load these translations in a different order:

'(function(d){' +
    `d['${ language }']=Object.assign(` +
        `d['${ language }']||{},` +
        `${ stringifiedTranslations }` +
    ')' +
'})(window.CKEDITOR_TRANSLATIONS||(window.CKEDITOR_TRANSLATIONS={}));'

It slightly increases the size of assets, but allows to use both approaches:

<script src="translations/pl.js">
<script>requirejs( [ 'ckeditor.js' ], ClassicEditor => {} )</script>

<script>requirejs( [ 'ckeditor.js', 'translations/pl.js' ], ClassicEditor => {} )</script>

TranslationService (ckeditor5-utils)

Builds and samples:

We can improve our samples and add UMD sample and translation sample. (These samples will only need HTML files and it's not possible to create them using manual tests.

ma2ciek on 20 Mar 2018

👍2

@Reinmar , @ma2ciek Thanks! I'm testing on the demo site of elFinder to edit the HTML file. It seems work perfectly!

nao-pon on 20 Mar 2018

I'm glad that these changes work for you well :) They should land in the next beta version.

ma2ciek on 21 Mar 2018

👍1

Your changes look really good. And kudos for the post with the summary.

I've got just two questions:

Will the language files be transpiled if Babel is enabled? I'm asking because you used an arrow function there. In other words, we need a final answer for https://github.com/ckeditor/ckeditor5-dev/pull/385/files#diff-c7651cbe9bf62ae0195e3db361131b75R216
Could you add in ckeditor5-build-classic (only) those samples using Require.JS? Perhaps there's some way to add them in tests/? I think that the manual test server doesn't look for .html files anyway, so we can add them there and simply write plain-HTML pages (kinda like sample/index.html). We'll use them only on special occasions anyway.

Reinmar on 21 Mar 2018

I think that the manual test server doesn't look for .html files anyway

It does. If the *.js file exists, corresponding *.html and *.md files also have to exist.

pomek on 22 Mar 2018

It does. If the *.js file exists, corresponding *.html and *.md files also have to exist.

But we don't need a JS file in this case. To make a sample for testing requirejs we need a simple HTML file with no assets (requirejs can be loaded from CDN and CKEditor 5 build from the ../build/ directory).

Reinmar on 22 Mar 2018

👍1

Right.

pomek on 22 Mar 2018

Could you add in ckeditor5-build-classic (only) those samples using Require.JS? Perhaps there's some way to add them in tests/? I think that the manual test server doesn't look for .html files anyway, so we can add them there and simply write plain-HTML pages (kinda like sample/index.html). We'll use them only on special occasions anyway.

Manual tests should depend on the HTML files and the rest should be optional to make it work. The second problem is that with the current implementation all files are moved or compiled to ckeditor5/build/.manual-tests directory. We'd have to somehow copy there the CKEditor5's builds, so the whole process would have to be smarter.

Will the language files be transpiled if Babel is enabled? I'm asking because you used an arrow function there. In other words, we need a final answer for ckeditor/ckeditor5-dev/pull/385/files#diff-c7651cbe9bf62ae0195e3db361131b75R216

Right, I almost forgot about it. Webpack's loaders won't touch this code so we should provide somehow the transpiled version if somebody uses babel etc. I think we can achieve this either by:

providing by default the ES5 version - translation files will grow a bit.
launching loaders that match JS files on these files - that would be tricky and dangerous
specifying the version of the ES in CKEdtirorWebpackPlugin, so we can use much shorter (ES6) version by default, but will have the ES5 version available too if somebody needs it.

ma2ciek on 22 Mar 2018

providing by default the ES5 version - translation files will grow a bit.

AFAICS, each language file (in a build) will contain just one function() call, so the difference is completely negligible. Let's use ES5-safe version by default.

Manual tests should depend on the HTML files and the rest should be optional to make it work. The second problem is that with the current implementation all files are moved or compiled to ckeditor5/build/.manual-tests directory. We'd have to somehow copy there the CKEditor5's builds, so the whole process would have to be smarter.

It can be simpler than that. We don't need to use the test:manual script for everything. To satisfy the current case we simply need to find a place in a build's directory structure to place a test HTML file which will be ignored when publishing this to npm. A good place for that is the tests/ directory. The fact that test:manual isn't able to build that is not a big problem – it's not perfect, but you can use your local HTTP server or whatever to open it in a browser. To be clear, it'd be this for me:

http://localhost/ckeditor5/packages/ckeditor5-build-classic/tests/<whatever-we-need-here>/sample-with-requirejs.html

Reinmar on 26 Mar 2018

It can be simpler than that.

To make it clear – let's not overcomplicate this. We need one test file that we'll use very rarely for this specific case. If we'll find out that this is a recurring problem we can think about a more general solution.

Reinmar on 26 Mar 2018

The new version of @ckeditor/ckeditor5-dev-webpack-plugin will fix this issue. However, it will only be compatible with beta.2 of CKEditor 5 so we need to wait a bit with releasing it.

Besides, I added two manual tests in ckeditor5-build-classic – theses tests are only accessible through a local HTTP server. Manual tests server doesn't pick them up.

Reinmar on 27 Mar 2018

OK, beta.2 (and beta.3 in case of builds) is out and builds will work fine with RequireJS now.

Reinmar on 10 Apr 2018

👍 OK, Thanks! but CDN items (ex. https://cdn.ckeditor.com/ckeditor5/1.0.0-beta.2/classic/ckeditor.js) are not found.

nao-pon on 11 Apr 2018

Turns out that CDN isn't updated yet. Working on this.

Reinmar on 11 Apr 2018

Was this page helpful?

0 / 5 - 0 ratings

Related issues

Drop-down looks misaligned

pjasiun · 3Comments

Should ImageButton be included in the builds?

Reinmar · 3Comments

How to enable SimpleUploadAdapter on this build?

metalelf0 · 3Comments

I want to set the default input font color. What should I do with this change?

pandora-iuz · 3Comments

The "Bold" icon is not centered

Reinmar · 3Comments