-
Notifications
You must be signed in to change notification settings - Fork 7
[#69553] Remove jQuery from CKEditor build #107
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
Conversation
- Replace jQuery.each() with native forEach() in button-disabler.js - Replace jQuery.getJSON() with fetch() API in user-mentions.js and work-package-mentions.js - Replace jQuery.ajax() with fetch() API in op-preview.plugin.js - Replace jQuery DOM manipulation with native DOM APIs in op-preview.plugin.js and op-source-code.plugin.js - All tests pass, build succeeds, and linter passes Co-authored-by: myabc <[email protected]>
Co-authored-by: myabc <[email protected]>
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.
Pull request overview
This pull request removes jQuery as a dependency from the CKEditor build, replacing all jQuery usage with vanilla JavaScript equivalents using modern DOM APIs and the Fetch API.
Key changes:
- Replaced jQuery AJAX methods (
$.getJSON(),$.ajax()) with nativefetch()API - Converted jQuery DOM manipulation to native DOM APIs (
document.createElement(),parentElement,querySelectorAll(), etc.) - Replaced
jQuery.each()with nativeArray.forEach()
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/op-source-code.plugin.js | Converted jQuery DOM traversal and manipulation to vanilla JS for source code mode toggling |
| src/plugins/op-preview.plugin.js | Replaced jQuery AJAX with fetch API and converted DOM manipulation for preview functionality |
| src/mentions/work-package-mentions.js | Replaced jQuery.getJSON() with fetch API for work package autocomplete |
| src/mentions/user-mentions.js | Replaced jQuery.getJSON() with fetch API for user mentions autocomplete |
| src/helpers/button-disabler.js | Converted jQuery.each() to native Array.forEach() for toolbar item iteration |
| README.md | Added migration notes warning downstream consumers to keep jQuery global available |
| CHANGELOG.md | Documented the jQuery removal with implementation details and migration guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/plugins/op-source-code.plugin.js
Outdated
| sourceWrapper.className = 'ck-editor__source'; | ||
|
|
||
| // Remove existing source elements | ||
| const existingSources = reference.parentElement.querySelectorAll('.ck-editor__source'); |
Copilot
AI
Dec 3, 2025
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.
The selector logic has changed from the original jQuery behavior. $reference.siblings('.ck-editor__source') only selected sibling elements of reference, but reference.parentElement.querySelectorAll('.ck-editor__source') selects ALL descendants matching the selector within the parent element, not just siblings. This could remove unintended elements if there are nested structures.
Consider using a more precise selector or filtering to match only siblings:
const existingSources = Array.from(reference.parentElement.children)
.filter(el => el.classList.contains('ck-editor__source'));
existingSources.forEach(el => el.remove());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.
@copilot please fix
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 in 0497dc1 - replaced querySelectorAll with Array.from(parentElement.children).filter() to match jQuery's .siblings() behavior
src/plugins/op-preview.plugin.js
Outdated
| const editableElement = editor.ui.getEditableElement(); | ||
| const reference = editableElement.parentElement; |
Copilot
AI
Dec 3, 2025
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.
Potential null reference error if editableElement.parentElement is null. The code immediately accesses reference.parentElement on lines 39 and 46 without checking if reference (which is editableElement.parentElement) is null first.
Consider adding a null check:
const editableElement = editor.ui.getEditableElement();
const reference = editableElement.parentElement;
if (!reference || !reference.parentElement) {
console.error('Cannot show preview: invalid editor structure');
return;
}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.
@copilot apply suggestion, but use optional chaining
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 in 0497dc1 - added optional chaining (?.) and early return guard with error logging
src/plugins/op-preview.plugin.js
Outdated
| let disablePreviewing = function() { | ||
| let $mainEditor = jQuery(editor.ui.getEditableElement()).parent(); | ||
| const editableElement = editor.ui.getEditableElement(); | ||
| const mainEditor = editableElement.parentElement; |
Copilot
AI
Dec 3, 2025
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.
Potential null reference error if editableElement.parentElement is null. The code immediately accesses mainEditor.parentElement on line 83 without checking if mainEditor (which is editableElement.parentElement) is null first.
Consider adding a null check:
const editableElement = editor.ui.getEditableElement();
const mainEditor = editableElement.parentElement;
if (!mainEditor || !mainEditor.parentElement) {
console.error('Cannot disable preview: invalid editor structure');
return;
}| const mainEditor = editableElement.parentElement; | |
| const mainEditor = editableElement && editableElement.parentElement; | |
| if (!mainEditor || !mainEditor.parentElement) { | |
| console.error('Cannot disable preview: invalid editor structure'); | |
| return; | |
| } |
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.
@copilot apply suggestion, but use optional chaining
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 in 0497dc1 - added optional chaining (?.) and early return guard with error logging as requested
src/plugins/op-source-code.plugin.js
Outdated
| const editableElement = editor.ui.getEditableElement(); | ||
| const reference = editableElement.parentElement; |
Copilot
AI
Dec 3, 2025
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.
Potential null reference error if editableElement.parentElement is null. The code immediately accesses reference.parentElement on lines 44 and 48 without checking if reference (which is editableElement.parentElement) is null first.
Consider adding a null check:
const editableElement = editor.ui.getEditableElement();
const reference = editableElement.parentElement;
if (!reference || !reference.parentElement) {
console.error('Cannot show source: invalid editor structure');
return;
}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.
@copilot apply suggestion, but use optional chaining
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 in 0497dc1 - added optional chaining (?.) and early return guard with error logging
src/plugins/op-source-code.plugin.js
Outdated
| let hideSource = function() { | ||
| let $mainEditor = jQuery(editor.ui.getEditableElement()).parent(); | ||
| const editableElement = editor.ui.getEditableElement(); | ||
| const mainEditor = editableElement.parentElement; |
Copilot
AI
Dec 3, 2025
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.
Potential null reference error if editableElement.parentElement is null. The code immediately accesses mainEditor.parentElement on line 70 without checking if mainEditor (which is editableElement.parentElement) is null first.
Consider adding a null check:
const editableElement = editor.ui.getEditableElement();
const mainEditor = editableElement.parentElement;
if (!mainEditor || !mainEditor.parentElement) {
console.error('Cannot hide source: invalid editor structure');
return;
}| const mainEditor = editableElement.parentElement; | |
| const mainEditor = editableElement.parentElement; | |
| if (!mainEditor || !mainEditor.parentElement) { | |
| console.error('Cannot hide source: invalid editor structure'); | |
| return; | |
| } |
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.
@copilot apply suggestion, but use optional chaining
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 in 0497dc1 - added optional chaining (?.) and early return guard with error logging
Co-authored-by: Copilot <[email protected]>
- Add optional chaining (?.) for null-safe property access as requested by @myabc - Add early return guards with error logging for invalid DOM structures - Fix showSource to use Array.from().filter() to match jQuery .siblings() behavior - All tests passing, linter passing Co-authored-by: myabc <[email protected]>
- Add `el !== mainEditor` check in disablePreviewing and hideSource filters - This ensures consistency with showPreview and showSource logic - Prevents potential edge case where mainEditor could be removed if it had the class Co-authored-by: myabc <[email protected]>
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.
Nice one 👍🏾 I place my confidence in the existing unit tests but I suppose some stuff might break, so we should probably let it hang in dev/edge sooner rather than later.
Built from upstream commit: 122e338255f254940dc02a5c2e72681e8d9c5d37 See upstream PRs: - opf/commonmark-ckeditor-build#107 - opf/commonmark-ckeditor-build#108
Built from upstream commit: 509e73464638e45f84d0b713089ea31b8cfb3ced See upstream PRs: - opf/commonmark-ckeditor-build#107 - opf/commonmark-ckeditor-build#108
Built from upstream commit: 7795208016d3efbf1cd8545aa852824fd2f3b149 See upstream PRs: - opf/commonmark-ckeditor-build#107 - opf/commonmark-ckeditor-build#108
Built from upstream commit: 7795208016d3efbf1cd8545aa852824fd2f3b149 See upstream PRs: - opf/commonmark-ckeditor-build#107 - opf/commonmark-ckeditor-build#108
Built from upstream commit: ec8c1b8b1a0a858467fdd4f15a761a25fd85f801 See upstream PRs: - opf/commonmark-ckeditor-build#107 - opf/commonmark-ckeditor-build#108
Built from upstream commit: ec8c1b8b1a0a858467fdd4f15a761a25fd85f801 See upstream PRs: - opf/commonmark-ckeditor-build#107 - opf/commonmark-ckeditor-build#108
Ticket
https://community.openproject.org/wp/69553
jQuery Removal - Complete
This PR removes all direct jQuery usage from the commonmark-ckeditor-build repository.
Changes Made
jQuery.each()with nativeArray.forEach()in button-disabler.jsjQuery.getJSON()withfetch()API in user-mentions.js and work-package-mentions.jsjQuery.ajax()withfetch()API in op-preview.plugin.jsFiles Changed (7 files)
src/helpers/button-disabler.js- Replaced jQuery.each with forEachsrc/mentions/user-mentions.js- Replaced jQuery.getJSON with fetchsrc/mentions/work-package-mentions.js- Replaced jQuery.getJSON with fetchsrc/plugins/op-preview.plugin.js- Replaced jQuery.ajax, DOM manipulation, improved error handlingsrc/plugins/op-source-code.plugin.js- Replaced jQuery DOM manipulation, added null checksCHANGELOG.md- Added (new file)README.md- Updated with migration notesTesting & Verification
✅ All 219 tests pass without modifications
✅ Build succeeds (webpack compiled successfully)
✅ ESLint passes with no errors
✅ All code review comments addressed
✅ No new issues in final code review
Migration Notes for Downstream Consumers
This library no longer uses jQuery internally. However, downstream applications (OpenProject) should continue to expose the jQuery global until other components are migrated.
Related downstream PR: opf/openproject#19429
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.