Expose API notifying about loaded CKEDITOR namespace #150
Conversation
sculpt0r
changed the title
There was a problem hiding this comment.
I'm thinking if we should remove all tests. They are only for getEditorNamespace. Now it's in ckeditor4-integrations-common so all tests should be performed there?
Yes, these tests should be removed, so we won't duplicate them in the ckeditor4-integrations-common repository. However, we will need additional tests confirming that the callback is working as expected. Please, see ckeditor/ckeditor4-vue#66 for some tips about tests we are looking for. At least, we should have test confirming that:
onNamespaceLoadedis executed and allows to changeCKEDITORnamespace on the initial CDN loadingonNamespaceLoadedis called only once for multiple editor instances, so you can't accidentally overwrite settings
We would also need some manual (aka sample) test confirming that the callback is executed just in a normal, browser environment (adding console log like in Vue samples should be fine).
| @@ -139,7 +139,8 @@ CKEditor.propTypes = { | |||
| name: PropTypes.string, | |||
| style: PropTypes.object, | |||
| readOnly: PropTypes.bool, | |||
| onBeforeLoad: PropTypes.func | |||
| onBeforeLoad: PropTypes.func, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
In Color nrmalizer we add @deprecated even if PR targeting major release.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/ckeditor.jsx
Outdated
| @@ -55,7 +55,7 @@ class CKEditor extends React.Component { | |||
| const { config, readOnly, type, onBeforeLoad, style, data } = this.props; | |||
| config.readOnly = readOnly; | |||
|
|
|||
| getEditorNamespace( CKEditor.editorUrl ).then( CKEDITOR => { | |||
| getEditorNamespace( CKEditor.editorUrl, this.props.onNamespaceLoaded ).then( CKEDITOR => { | |||
There was a problem hiding this comment.
Since we are already destructing props at the beginning of the method (55), maybe let's stick to it and destruct onNamespaceLoaded also?
jacekbogdanski
changed the title
| @@ -139,7 +139,8 @@ CKEditor.propTypes = { | |||
| name: PropTypes.string, | |||
| style: PropTypes.object, | |||
| readOnly: PropTypes.bool, | |||
| onBeforeLoad: PropTypes.func | |||
| onBeforeLoad: PropTypes.func, | |||
There was a problem hiding this comment.
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.
Moved to
ckeditor4-integrations-commongetEditorNamespace. Allow user passonNamespaceLoadedfunction with props.Removed failing test. Since getEditorNamespace is at common repo - add there verification tests:
ckeditor/ckeditor4-integrations-common#11
I'm thinking if we should remove all tests. They are only for getEditorNamespace. Now it's in
ckeditor4-integrations-commonso all tests should be performed there?Closes #129