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

Expose API notifying about loaded CKEDITOR namespace #150

Merged
merged 11 commits into from
Dec 7, 2020
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
New Features:

* [#125](https://github.com/ckeditor/ckeditor4-react/issues/125): Added `name` property for easier referencing editor instances with [`CKEDITOR.instances`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR.html#property-instances). Thanks to [Julien Castelain](https://github.com/julien)!
* [#129](https://github.com/ckeditor/ckeditor4-react/issues/129): Added `onNamespaceLoaded` property executed when [`CKEDITOR` namespace](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR.html) is loaded, which can be used for its easier customization.

## ckeditor4-react 1.2.1

Expand Down
9 changes: 9 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"babel-loader": "^8.0.4",
"chai": "^4.2.0",
"ckeditor4": "^4.15.1",
"ckeditor4-integrations-common": "^0.1.0",
"core-js": "^3.6.4",
"enzyme": "^3.7.0",
"enzyme-adapter-react-16": "^1.6.0",
Expand Down
36 changes: 22 additions & 14 deletions samples/main.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
var { useState } = React;
var { HashRouter, Switch, Route, Link } = ReactRouterDOM;
const { useState } = React;
const { HashRouter, Switch, Route, Link } = ReactRouterDOM;

var startingData = '<p>This is a CKEditor 4 WYSIWYG editor instance created by ️⚛️ React.</p>';
const startingData = '<p>This is a CKEditor 4 WYSIWYG editor instance created by ️⚛️ React.</p>';

ReactDOM.render(
<HashRouter>
Expand All @@ -13,7 +13,7 @@ ReactDOM.render(

function Navigation() {
return (
<ul class='routes'>
<ul className='routes'>
<li>
<Link to="/">Editor Types</Link>
<Link to="/events">Component Events</Link>
Expand All @@ -40,7 +40,7 @@ function Router() {
}

function EditorTypes() {
var [ readOnly, setReadOnly ] = useState( false );
const [ readOnly, setReadOnly ] = useState( false );

return (
<main>
Expand All @@ -51,6 +51,7 @@ function EditorTypes() {
type="classic"
data={ startingData }
readOnly={ readOnly }
onNamespaceLoaded={ onNamespaceLoaded }
/>
</section>
<section>
Expand All @@ -59,6 +60,7 @@ function EditorTypes() {
type="inline"
data={ startingData }
readOnly={ readOnly }
onNamespaceLoaded={ onNamespaceLoaded }
/>
</section>

Expand All @@ -77,7 +79,7 @@ function EditorTypes() {
}

function EditorEvents() {
var [ events, setEvents ] = useState( [] );
const [ events, setEvents ] = useState( [] );

return (
<main>
Expand All @@ -90,6 +92,7 @@ function EditorEvents() {
onBlur={ logEvent }
onChange={ logEvent }
onSelectionChange={ logEvent }
onNamespaceLoaded={ onNamespaceLoaded }
/>
<h3>Events Log</h3>
<p><small>To check additional details about every event, consult the console in the browser developer tools.</small></p>
Expand All @@ -116,7 +119,7 @@ function EditorEvents() {
}
}

function EventLog ( { stream } ) {
function EventLog( { stream } ) {
return (
<div className="event-log">
<ul>
Expand All @@ -135,8 +138,8 @@ function EventLog ( { stream } ) {
}

function EditorTwoWayDataBinding() {
var [ data, setData ] = useState( startingData ),
[ sourceEditorFocused, setSourceEditorFocus ] = useState( false );
const [ data, setData ] = useState( startingData );
const [ sourceEditorFocused, setSourceEditorFocus ] = useState( false );

return (
<div>
Expand All @@ -159,6 +162,7 @@ function EditorTwoWayDataBinding() {
<CKEditor
data={ data }
onChange={ ( { editor } ) => setData( editor.getData() ) }
onNamespaceLoaded={ onNamespaceLoaded }
/>

<EditorPreview data={ data } />
Expand All @@ -168,9 +172,9 @@ function EditorTwoWayDataBinding() {
}

function SourceEditor( { data, textHandler, focused, setFocus } ) {
var textareaValue = {},
// Updating editor on every keystroke may freeze a browser.
onChange = debounce( textHandler, 300 );
const textareaValue = {};
// Updating editor on every keystroke may freeze a browser.
const onChange = debounce( textHandler, 300 );

// Value should be only updated if source editor is not focused.
// Otherwise it will interrupt typing with new data set.
Expand Down Expand Up @@ -203,10 +207,10 @@ function EditorPreview( { data } ) {
}

function debounce( fn, wait ) {
var timeout;
let timeout;

return function( ...args ) {
var context = this;
const context = this;

clearTimeout( timeout );

Expand All @@ -215,3 +219,7 @@ function debounce( fn, wait ) {
}, wait );
};
}

function onNamespaceLoaded( namespace ) {
console.log( 'CKEDITOR version: ', namespace.version );
}
9 changes: 5 additions & 4 deletions src/ckeditor.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import React from 'react';
import PropTypes from 'prop-types';
import getEditorNamespace from './getEditorNamespace.js';
import { getEditorNamespace } from 'ckeditor4-integrations-common';

class CKEditor extends React.Component {
constructor( props ) {
Expand Down Expand Up @@ -52,10 +52,10 @@ class CKEditor extends React.Component {
}

_initEditor() {
const { config, readOnly, type, onBeforeLoad, style, data } = this.props;
const { config, readOnly, type, onBeforeLoad, onNamespaceLoaded, style, data } = this.props;
config.readOnly = readOnly;

getEditorNamespace( CKEditor.editorUrl ).then( CKEDITOR => {
getEditorNamespace( CKEditor.editorUrl, onNamespaceLoaded ).then( CKEDITOR => {
// (#94)
if ( this._destroyed ) {
return;
Expand Down Expand Up @@ -139,7 +139,8 @@ CKEditor.propTypes = {
name: PropTypes.string,
style: PropTypes.object,
readOnly: PropTypes.bool,
onBeforeLoad: PropTypes.func
onBeforeLoad: PropTypes.func,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should still keep onBeforeLoad. It seems to be not documented anywhere, on the other side we already see that someone could be using it: #129 (comment)

It would be good to keep API consistent with what we have in Vue and Angular, also this callback may create unexpected results described in #129 (comment)

Not sure if we should go with a deprecation message or just get rid of it and target this PR into a major release. @Comandeer do you have any thoughts on that issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Color nrmalizer we add @deprecated even if PR targeting major release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should still keep onBeforeLoad. It seems to be not documented anywhere, on the other side we already see that someone could be using it: #129 (comment)

It would be good to keep API consistent with what we have in Vue and Angular, also this callback may create unexpected results described in #129 (comment)

Founded: #47. Seems that the initial driver for adding onBeforeLoad callback is in contradiction with #129 (comment)

So we have onBeforeLoad - per instance call nad onNamespaceLoaded - call before all instances loaded and it's called once.

To me: those are two different behaviours which, however, may lead to confusion if used improperly. So maybe good documentation may help here? So I would stay with both callbacks and eventually mark onBeforeLoad with @deprecated if we would like to keep API consistent with others integrations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note about deprecating stuff and versions - in CKEditor 4 we don't really follow semver - we have only minor and major releases there. CKEditor 4 minor is really a patch in semver nomenclature (bug fixes only) while CKEditor 4 major is minor in semver (new features). As for semver major which means breaking changes we really avoid such in CKEditor 4.

So according to semver (which we follow here), getting rid of onBeforeLoad entirely means breaking changes and it should be introduced in major. While deprecating stuff should be done in minor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking this out @sculpt0r. Looks like we have 2 problems: this property has been added by the community, so we can expect it's used and it has not been documented by us.

Let's keep it, for now, I will create a separate ticket for documentation and we will decide later what to do with duplicated logic. I would be for adding information to docs that "it's recommended to use onNamespaceLoaded and onBeforeLoad is deprecated or could be a "subject of deprecation", but it's something worth discussing when writing docs.

onNamespaceLoaded: PropTypes.func
};

CKEditor.defaultProps = {
Expand Down
33 changes: 0 additions & 33 deletions src/getEditorNamespace.js

This file was deleted.

68 changes: 16 additions & 52 deletions tests/browser/ckeditor.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,14 @@ import { configure, mount } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';

import CKEditor from '../../src/ckeditor.jsx';
import getEditorNamespace from '../../src/getEditorNamespace.js';
import { getEditorNamespace } from 'ckeditor4-integrations-common';

const { expect, use } = chai;
use( sinonChai );
configure( { adapter: new Adapter() } );

let components = [];

CKEditor.editorUrl = 'https://cdn.ckeditor.com/4.13.1/standard-all/ckeditor.js';

describe( 'CKEditor Component', () => {
let sandbox;

Expand Down Expand Up @@ -495,66 +493,32 @@ describe( 'CKEditor Component', () => {
} );

describe( 'getEditorNamespace', () => {
// Don't need any files – Data URLs FTW!
const fakeScript = 'data:text/javascript;base64,d2luZG93LkNLRURJVE9SID0ge307';

beforeEach( () => {
delete window.CKEDITOR;
} );

it( 'is a function', () => {
expect( getEditorNamespace ).to.be.a( 'function' );
} );

it( 'requires non-empty string as parameter', () => {
const invalid = [
1,
{},
[],
null,
undefined,
''
];

invalid.forEach( param => {
expect( () => {
getEditorNamespace( param );
} ).to.throw( TypeError, 'CKEditor URL must be a non-empty string.' );
} );
} );

it( 'returns promise even if namespace is present', () => {
window.CKEDITOR = {};
it( 'called once for multiply editor instances', () => {
const spy = sinon.spy();

const namespacePromise = getEditorNamespace( 'test' );
expect( namespacePromise ).to.be.an.instanceOf( Promise );

return namespacePromise.then( namespace => {
expect( namespace ).to.equal( window.CKEDITOR );
} );
} );

it( 'load script and resolves with loaded namespace', () => {
return getEditorNamespace( fakeScript ).then( namespace => {
expect( namespace ).to.equal( window.CKEDITOR );
} );
} );

it( 'returns the same promise', () => {
const promise1 = getEditorNamespace( fakeScript );
const promise2 = getEditorNamespace( fakeScript );
const editor1 = createEditor( { name: 'editor-name-one', onNamespaceLoaded: spy } );
const editor2 = createEditor( { name: 'editor-name-two', onNamespaceLoaded: spy } );
const editor3 = createEditor( { name: 'editor-name-three', onNamespaceLoaded: spy } );

return Promise.all( [
promise1,
promise2
waitForEditors( [ editor1, editor2, editor3 ] )
] ).then( () => {
expect( promise1 ).to.equal( promise2 );
expect( spy.calledOnce ).to.be.equal( true );
} );
} );

it( 'rejects with error when script cannot be loaded', () => {
return getEditorNamespace( 'non-existent.js' ).catch( err => {
expect( err ).to.be.instanceOf( Error );
it( 'after resolve allows to change CKEDITOR namespace', () => {
const language = 'custom';
const editor = createEditor( { name: 'editor-name', onNamespaceLoaded: namespace => {
namespace.config.lang = language;
} } );

waitForEditor( editor ).then( () => {
expect( CKEDITOR.config.lang ).to.be.equal( language );
} );
} );
} );
Expand Down