t/5: Introducing the CKEDITOR namespace, the AMD API and the Builder. #6
Conversation
… loading so the "ckeditor" module can be properly called from outside.
… the RequireJS "plugin!" module.
…so the basePath computation code is also shared.
|
|
||
| function cleanup( callback ) { | ||
| var del = require( 'del' ); | ||
| del.sync( target ); |
There was a problem hiding this comment.
Why synchronously when you have a callback to call when ready?
There was a problem hiding this comment.
Just to make the code simpler, as we have another call right after that. In any case we need to wait for you anyway, so it should not be a big del, right?
There was a problem hiding this comment.
should not be a big del
Right ^_^
I think that, to properly handle PRs, we should avoid force-pushes so we'll have to live this as it is. In any case, in both points the code work. Those are just small details.
Ok, I've removed those comments as we should not really have them at this stage. |
|
@gregpabian, I've fixed all your remarks with the above commits. |
| preserveLicenseComments: false, | ||
| wrap: { | ||
| startFile: [ 'dev/tasks/build/start.frag', require.resolve( 'almond' ) ], | ||
| endFile: 'dev/tasks/build/end.frag' |
There was a problem hiding this comment.
I think we should put start and end fragments somewhere near the sources, not in dev/tasks/build. Eg. jQuery uses src/intro.js, src/outro.js, angular uses eg. src/module.prefix.
There was a problem hiding this comment.
+1 from me because those are closer to cource code then to tasks source code/configuration.
Be careful with extension. The .frag is good because it can be white listed on jshint/jscss and in IDE/text editor. Those files can be "invalid" like a function that is cut in half and put in two separate files.
There was a problem hiding this comment.
It makes sense. I've moved them with 76d64c6.
| // * CKEditor source code | ||
| // * end.frag | ||
|
|
||
| ( function ( root, factory ) { |
There was a problem hiding this comment.
There should be no space after function.
|
|
||
| // Before starting, run the initial checkups. | ||
| if ( !this.checkUp() ) { | ||
| console.log( 'Build operation aborted.' ); |
There was a problem hiding this comment.
You can use http://gruntjs.com/api/grunt.log - e.g. grunt.error.log.
BTW. I think I've read somewhere that using console.log in Node.JS for something more than debugging isn't good. @gregpabian: Do you confirm?
There was a problem hiding this comment.
AFAIR it was because of the process output. If you use proc.stdout/stderr you can pipe or redirect this, while with console.log you can't.
There was a problem hiding this comment.
Not really - console.log is just a wrapper around stdout.write so the logs will be redirected if we want to.
There was a problem hiding this comment.
Additionally, the class is coded in a way that it is independent from grunt so it can use by pure node code as well.
There was a problem hiding this comment.
So I remembered something wrong. Just one thing then - console.error() is IMO more appropriate.
| * @property {Array} | ||
| */ | ||
| this.taskList = [ | ||
| [ 'cleanUp', 'Cleaning the "' + target + '" directory...' ], |
There was a problem hiding this comment.
Now (49b17a2) I've got:
Running "build" task
1. Cleaning the "undefined" directory...
BTW. I miss very much an information about where the build was created - where should I look for it now.
|
|
||
| // Make the build an AMD module. | ||
| if ( typeof define == 'function' && define.amd ) { | ||
| define( [], function() { |
There was a problem hiding this comment.
http://requirejs.org/docs/api.html#deffunc - the array seems to be unnecessary.
| var replace = require( 'replace' ); | ||
|
|
||
| replace( { | ||
| regex: /^\s*CKEDITOR\.(define|require)/mg, |
There was a problem hiding this comment.
Do we have to do this? Can't the minified code be using define and require through the CKEDITOR namespace?
There was a problem hiding this comment.
This is necessary for r.js to find dependencies. Actually, to find modules and theirs deps.
|
Hurray! The end of the review marathon :D PS. Before merging please check 1a37506. |
|
I'll extract #6 (diff) to a new ticket. |
| return; | ||
| } | ||
|
|
||
| console.log( 'Building CKEditor into the "' + this.target + '" directory:') |
There was a problem hiding this comment.
JSHint and JSCS throw errors here - missing semicolon and missing space before closing round bracket.
There was a problem hiding this comment.
Fixed with cccf119.
Damn WebStorm that doesn't work with our pre-commit hook. They fixed it some good time ago but still it didn't land into any of their releases.
update ReadMe
The proposed code means to close #2, #4 and #5.
I've pushed t/2 and t/4 as well, but I'm putting t/5 only under review because all those features are extremely related. They're actually defining the basics for our AMD approach. So it is simpler to review t/5 only and close all those issues at once.
There is a cross-requirement for this PR in ckeditor5-core:
https://github.com/ckeditor/ckeditor5-core/issues/1
Therefore to review this one, ckeditor5-core must be at t/1.
The builder is simply merging the main scripts for now. Other features are to be added in the future so a complete and real distribution version will be created. For now, the focus is creating an interchangeable build version of the root
ckeditor.js.No tests are included because it was not possible to bring the infrastructure necessary for the test environment (#1) without any API to get tested. Therefore, tests will be added later once this PR is masterised and #1 gets closed. A ticket will be opened as a followup to this PR.
The builder can be executed by simply running
grunt build.