Enable ckeditor5 builds to work with RequireJS #385
Conversation
| const outputBody = `CKEDITOR_TRANSLATIONS.add('${ language }',${ stringifiedTranslations })`; | ||
| const outputBody = ( | ||
| // We need to ensure that the CKEDITOR_TRANSLATIONS variable exist and if it exists, we need to extend it. | ||
| '(function(d){' + |
There was a problem hiding this comment.
Do we add this bit after the code is minified?
There was a problem hiding this comment.
Hmm, I was thinking about someone that wants to use our builds and targets ES5. Otherwise, we can change it to arrow fn.
There was a problem hiding this comment.
I used Object.assign later 🤦♂️
There was a problem hiding this comment.
I replaced it with an arrow function. But targeting ES5 might be important, and this code won't be transpiled, it should be considered.
| const outputBody = `CKEDITOR_TRANSLATIONS.add('${ language }',${ stringifiedTranslations })`; | ||
| const outputBody = ( | ||
| // We need to ensure that the CKEDITOR_TRANSLATIONS variable exist and if it exists, we need to extend it. | ||
| '(d=>' + |
There was a problem hiding this comment.
I didn't mean – why it was a function? I meant – why it's minified? I'm completely ok with function if you know that this code won't be transpiled. But I'm less ok with manually minifying the code (which makes it hard to read ;P).
There was a problem hiding this comment.
Because it won't be transpiled later. It's added to the built asset. And we want the minified code in the production mode.
There was a problem hiding this comment.
Hmm. Maybe I'm a little bit wrong here. After changing listening to emit event to listening to optimize-chunk-assets it might be optimized later. I'll check it.
There was a problem hiding this comment.
Yes. In fact, transpilation is done later, but it breaks sourcemaps. I'll try to work-around this quickly.
|
OK. I've found few interesting things digging more into webpack code.
If we want to have nice looking code that you've proposed #385 (review), than we will need to minify all assets, including translation assets after adding some minifier. But that will also lead to emitting map files (as have source-map enabled) and adding license banner to each translation file - and I'm not sure if we want it. |
|
To make it clear, IMO it's best to provide already minified translations and do not touch compilation chunks as other plugins can modify related files and we actually don't want it. |
|
Thanks for clarification. It's fine. |
ma2ciek
force-pushed
the
t/ckeditor5/914
branch
from
696b517 to
5893351
Compare
|
I've tested it with targeting one language and multiple language and sourcemaps should work fine now. I've updated builds on the |
|
R- for reasons explained in ckeditor/ckeditor5#914 (comment) |
|
I'll finish this PR. Reassigning. |
| `d['${ language }']||{},` + | ||
| `${ stringifiedTranslations }` + | ||
| ')' + | ||
| '})(window.CKEDITOR_TRANSLATIONS||(window.CKEDITOR_TRANSLATIONS={}));' |
There was a problem hiding this comment.
I think that this kind of access to the global object (through window) is going to hurt in some odd envs, but let's keep it for now.
| // We need to ensure that the CKEDITOR_TRANSLATIONS variable exists and if it exists, we need to extend it. | ||
| // Use ES5 because this bit will not be transpiled! | ||
| '(function(d){' + | ||
| `d['${ language }']=Object.assign(` + |
There was a problem hiding this comment.
It's still an ES6 code. We can change it to for in loop to be fully ES5 compatible.
There was a problem hiding this comment.
Object.assign() needs to be provided by a polyfill for the rest of the code anyway. This doesn't get transpiled.
There was a problem hiding this comment.
This might be tricky for someone, as we add that code at the top of the bundle.
There was a problem hiding this comment.
Right now we don't support any non-ES6 env anyway, so it's even hard to test. Let's keep it in our minds. But I don't see a problem in injecting the ES6 polyfill before our scripts.
Suggested merge commit message (convention)
Fix: Enabled ckeditor5 builds to work with
RequireJS. Closes ckeditor/ckeditor5#914.Additional information
See ckeditor/ckeditor5#914 (comment).
Branches:
https://github.com/ckeditor/ckeditor5-utils/tree/t/ckeditor5/914
https://github.com/ckeditor/ckeditor5-build-classic/tree/t/ckeditor5/914
https://github.com/ckeditor/ckeditor5-build-inline/tree/t/ckeditor5/914
https://github.com/ckeditor/ckeditor5-build-balloon/tree/t/ckeditor5/914
https://github.com/ckeditor/ckeditor5-build-decoupled-document/tree/t/ckeditor5/914