-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
t/5: Introducing the CKEDITOR namespace, the AMD API and the Builder. #6
Changes from 15 commits
1c16b97
b44682c
03f78c4
2c6e785
a23bda8
1eddc34
9368232
e0ebc2c
ba2f8c6
130b2c7
a3a4956
1dce371
767f6ac
ef522dd
255d2b5
2f578cf
051574a
6113982
c2413f0
c3fc22c
667acb1
76d64c6
f4074f0
9a86ebb
10246ac
3d4fbbe
950871b
66297bb
49b17a2
8880b20
990f135
2c89d5d
ef856f4
1a37506
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,15 @@ Where not otherwise indicated, all CKEditor content is authored by CKSource engi | |
intellectual property. In some specific instances, CKEditor will incorporate work done by developers outside of CKSource | ||
with their express permission. | ||
|
||
(Ignore this line: %REMOVE_START%) | ||
|
||
Software available at our [repository](https://github.com/ckeditor/ckeditor5) and developer version only: | ||
|
||
- RequireJS (lib/requirejs) <br> | ||
Licensed under the terms of the MIT or new BSD license. <br> | ||
Copyright (c) 2010-2014, The Dojo Foundation All Rights Reserved. | ||
|
||
(Ignore this line: %REMOVE_END%) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you remove the first or last blank line inside the above block we will be able to add a blank line after it, so the next header will be visually separated from the contents of this section :P. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I didn't understand this one. Feel free to change it yourself. |
||
Trademarks | ||
---------- | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
/** | ||
* @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved. | ||
* For licensing, see LICENSE.md. | ||
*/ | ||
|
||
/* global requirejs, define, require, window, document, location */ | ||
|
||
'use strict'; | ||
|
||
// This file is shared by the dev and release versions of CKEditor. It bootstraps the API. | ||
|
||
( function( root ) { | ||
var CKEDITOR = root.CKEDITOR = { | ||
/** | ||
* The full URL for the CKEditor installation directory. | ||
* | ||
* It is possible to manually provide the base path by setting a global variable named `CKEDITOR_BASEPATH`. This | ||
* global variable must be set **before** the editor script loading. | ||
* | ||
* alert( CKEDITOR.basePath ); // e.g. 'http://www.example.com/ckeditor/' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with 10246ac. |
||
* | ||
* @property {String} | ||
*/ | ||
basePath: getBasePath(), | ||
|
||
/** | ||
* Defines an AMD module. | ||
* | ||
* See https://github.com/ckeditor/ckeditor5-design/wiki/AMD for more details about our AMD API. | ||
* | ||
* @method | ||
* @member CKEDITOR | ||
*/ | ||
define: define, | ||
|
||
/** | ||
* Retrieves one or more AMD modules. | ||
* | ||
* Note that the CKEditor AMD API does not download modules on demand so be sure to have their relative scripts | ||
* available in the page. | ||
* | ||
* See https://github.com/ckeditor/ckeditor5-design/wiki/AMD for more details about our AMD API. | ||
* | ||
* @method | ||
* @member CKEDITOR | ||
*/ | ||
require: require | ||
}; | ||
|
||
requirejs.config( { | ||
// Modules are generally relative to the core project. | ||
baseUrl: CKEDITOR.basePath + 'node_modules/ckeditor5-core/src/', | ||
|
||
// These configurations will make no difference in the build version because the following paths will be | ||
// already defined there. | ||
paths: { | ||
// Hide the core "ckeditor" under a different name. | ||
'ckeditor-core': CKEDITOR.basePath + 'node_modules/ckeditor5-core/src/ckeditor', | ||
|
||
// The dev version overrides for the "ckeditor" module. This is empty on release. | ||
'ckeditor-dev': CKEDITOR.basePath + 'src/ckeditor-dev' | ||
} | ||
} ); | ||
|
||
// Define a new "ckeditor" module, which overrides the core one with the above and the dev stuff. | ||
define( 'ckeditor', [ 'ckeditor-core', 'ckeditor-dev', 'utils' ], function( core, dev, utils ) { | ||
utils.extend( core, root.CKEDITOR, ( dev || {} ) ); | ||
root.CKEDITOR = core; | ||
|
||
return core; | ||
} ); | ||
|
||
function getBasePath() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function should be testable. We already had problem in CKEditor 4 that in order to test the base path we had to load multiple iframes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with 9a86ebb. |
||
if ( window.CKEDITOR_BASEPATH ) { | ||
return window.CKEDITOR_BASEPATH; | ||
} | ||
|
||
var scripts = document.getElementsByTagName( 'script' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why so much spacing between variable declarations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with 10246ac. |
||
|
||
var basePathSrcPattern = /(^|.*[\\\/])ckeditor\.js(?:\?.*|;.*)?$/i; | ||
|
||
var path; | ||
|
||
// Find the first script that src matches ckeditor.js. | ||
[].some.call( scripts, function( script ) { | ||
var match = script.src.match( basePathSrcPattern ); | ||
|
||
if ( match ) { | ||
path = match[ 1 ]; | ||
|
||
return true; | ||
} | ||
} ); | ||
|
||
if ( path.indexOf( ':/' ) == -1 && path.slice( 0, 2 ) != '//' ) { | ||
if ( path.indexOf( '/' ) === 0 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with 3d4fbbe. |
||
path = location.href.match( /^.*?:\/\/[^\/]*/ )[ 0 ] + path; | ||
} else { | ||
path = location.href.match( /^[^\?]*\/(?:)/ )[ 0 ] + path; | ||
} | ||
} | ||
|
||
return path; | ||
} | ||
} )( window ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,158 @@ | ||
/** | ||
* @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved. | ||
* For licensing, see LICENSE.md. | ||
*/ | ||
|
||
/* jshint node: true */ | ||
|
||
'use strict'; | ||
|
||
module.exports = function( grunt ) { | ||
grunt.registerTask( 'build', 'Build a release out of the current development code.', function() { | ||
var done = this.async(); | ||
module.exports.build( done ); | ||
} ); | ||
}; | ||
|
||
// Exports the build method so it can be used from plain node code as well. | ||
module.exports.build = function( done ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we create a class for the builder? And then wouldn't it be good to have unit tests for builder too? It's not super complicated right now, but there's no doubt that it will grow, and tdd would be very handy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good suggestion. I've just done that with c3fc22c.
I thought about that but I didn't want to spend time on this as there is no infrastructure for such tests in place. There are too many things to be bootstrapped right now, so I would propose to move ahead without tests here and make this a future enhancement. |
||
var target = 'build'; | ||
var tmp = 'tmp'; | ||
var stepCounter = 0; | ||
|
||
var tasks = [ | ||
[ cleanup, 'Cleaning the "' + target + '" directory...' ], | ||
[ copyToTmp, 'Copying source files for manipulation...' ], | ||
[ removeAmdNamespace, 'AMD cleanup...' ], | ||
[ optimize, 'Creating the optimized code...' ], | ||
[ cleanupTmp, 'Removing the "' + tmp + '" directory...' ] | ||
]; | ||
|
||
runNext(); | ||
|
||
function runNext() { | ||
var next = tasks.shift(); | ||
|
||
if ( next ) { | ||
stepCounter++; | ||
console.log( stepCounter + '. ' + next[ 1 ] ); | ||
next[ 0 ]( runNext ); | ||
} else { | ||
if ( done ) { | ||
done(); | ||
} | ||
} | ||
} | ||
|
||
function cleanup( callback ) { | ||
var del = require( 'del' ); | ||
del.sync( target ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why synchronously when you have a callback to call when ready? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right ^_^ |
||
del.sync( tmp ); | ||
|
||
return callback(); | ||
} | ||
|
||
function copyToTmp( callback ) { | ||
var ncp = require( 'ncp' ).ncp; | ||
var path = require( 'path' ); | ||
var fs = require( 'fs' ); | ||
|
||
var deps = JSON.parse( fs.readFileSync( 'package.json', 'utf8' ) ).dependencies; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use a plain var deps = require('../../package.json').dependencies; |
||
|
||
var toCopy = Object.keys( deps ).filter( function( name ) { | ||
return name.indexOf( 'ckeditor5-' ) === 0; | ||
} ); | ||
|
||
if ( !fs.existsSync( tmp ) ) { | ||
fs.mkdirSync( tmp ); | ||
} | ||
|
||
function copy() { | ||
var module = toCopy.shift(); | ||
|
||
if ( !module ) { | ||
return callback(); | ||
} | ||
|
||
var dest = path.join( tmp + '/', module ); | ||
|
||
if ( !fs.existsSync( dest ) ) { | ||
fs.mkdirSync( dest ); | ||
} | ||
|
||
// Copy the "src" directory only. | ||
ncp( path.join( 'node_modules', module, 'src' ), path.join( dest, 'src' ), { | ||
dereference: true | ||
}, function( err ) { | ||
if ( err ) { | ||
throw( err ); | ||
} | ||
|
||
copy(); | ||
} ); | ||
} | ||
|
||
copy(); | ||
} | ||
|
||
function removeAmdNamespace( callback ) { | ||
var replace = require( 'replace' ); | ||
|
||
replace( { | ||
regex: /^\s*CKEDITOR\.(define|require)/mg, | ||
replacement: '$1', | ||
paths: [ 'tmp' ], | ||
recursive: true, | ||
silent: true | ||
} ); | ||
|
||
callback(); | ||
} | ||
|
||
function optimize( callback ) { | ||
var requirejs = require( 'requirejs' ); | ||
|
||
var config = { | ||
out: target + '/ckeditor.js', | ||
|
||
baseUrl: tmp + '/ckeditor5-core/src/', | ||
paths: { | ||
'ckeditor': '../../../ckeditor', | ||
'ckeditor-dev': '../../../src/ckeditor-dev', | ||
'ckeditor-core': 'ckeditor' | ||
// depTree: '../../../lib/depTree', | ||
// plugins: '../../../lib/plugins' | ||
}, | ||
|
||
// include: ['depTree', 'ckeditor' ].concat( getPlugins() ), | ||
include: [ 'ckeditor' ], | ||
stubModules: [ 'ckeditor-dev' ], | ||
|
||
// onBuildWrite: replacePaths, | ||
// optimize: 'none', | ||
optimize: 'uglify2', | ||
//uglify2: { | ||
// output: { | ||
// beautify: true | ||
// }, | ||
// mangle: false | ||
//}, | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should put start and end fragments somewhere near the sources, not in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 from me because those are closer to cource code then to tasks source code/configuration. Be careful with extension. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense. I've moved them with 76d64c6. |
||
} | ||
}; | ||
|
||
requirejs.optimize( config, function() { | ||
callback(); | ||
} ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no need to wrap it in a function. |
||
} | ||
|
||
function cleanupTmp( callback ) { | ||
var del = require( 'del' ); | ||
del.sync( tmp ); | ||
|
||
return callback(); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
/************************ end.frag START */ | ||
|
||
return require( 'ckeditor' ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
( function ( root, factory ) { | ||
// Register the CKEDITOR global. | ||
root.CKEDITOR = factory(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we pollute the global namespace even though someone wants to use CKEditor5 as an AMD module? That's completely against AMD principles... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about that... but just the fact that For instance, developers may not even be developing with AMD at all, but maybe their CMSs are making Therefore, it sounded like the safest approach to enable both ways. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Safe doesn't always mean right. They can still expose the global namespace themselves. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand, but you must understand that this level of flexibility is required because we deal with all kinds of developers out there. Some would be prepared for AMD but many will just have troubles. Note that our goal is not making CKEditor an AMD module, but instead enable it as a AMD module to satisfy AMD devs. For the sake of being "right" we may even consider a configuration option for the builder where one could force "pure AMD", so no global is created. But by default we should go the "satisfy all" way for easy adoption. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, having this configurable might do the trick. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll open an issue for this once this PR goes accepted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found an interesting thing - https://github.com/jrburke/requirejs/wiki/Updating-existing-libraries#anon If I understand correctly, if This means that there must be (somehow available) a totally non-AMD version of The remaining questions are:
Ad 1. I think that 3 - AMD with polluting (could be the default), AMD without polluting (for pedants like many of us), no AMD at all (for concatenation). Ad 2. jQuery is doing the "AMD with polluting", so I think we can do it as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Reinmar, it makes sense. I think we can proceed with the existing code and the do what I already said, to avoid such important thoughts to be left inside commits comments.:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up: #8. |
||
|
||
// Make the build an AMD module. | ||
if ( typeof define == 'function' && define.amd ) { | ||
define( [], function() { | ||
return root.CKEDITOR; | ||
} ); | ||
} | ||
} )( this, function () { | ||
|
||
/************************ start.frag END */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
*
insteaf of-
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 10246ac.