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

[WIP] Upgrade CkEditor to v41 #9212

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

[WIP] Upgrade CkEditor to v41 #9212

wants to merge 32 commits into from

Conversation

jimrandomh
Copy link
Collaborator

@jimrandomh jimrandomh commented May 2, 2024

This upgrades CkEditor to v41. The basics are working including collaborative editing, but I need to test dialogues, LaTeX, and our other plugins more. There is one mysterious console error "collection-add-invalid-id".

In addition to the version upgrade, this enables both Typescript and eslint checking for our CkEditor plugins. In order to get Typescript to pass, I put in a lot of casts to "AnyBecauseTodo", and the stricter Typescript settings (noImplicitAny, strictNullChecks) are not yet turned on. I used the ckeditor5 eslint preset merged with our main repo preset, then turned off a lot of rules that the ckeditor5 preset had added (see comments in .esintrc.js).

Some of those casts to AnyBecauseTodo are covering things that look like they might be crash-bugs in our plugins. In particular, there are things that traverse the document and expect to find a particular element type (but don't check that they found what they expected), and event handlers that use their sender (but don't check that the event sender is what they expected). There were also a few functions that straight up passed completely incorrect arguments, eg calls to toWidget passing the string "div" in the place where an options object should be. I expect that tightening this up will improve the stability of our editor.

┆Issue is synchronized with this Asana task by Unito

@b0b3rt
Copy link
Collaborator

b0b3rt commented May 3, 2024

There were also a few functions that straight up passed completely incorrect arguments, eg calls to toWidget passing the string "div" in the place where an options object should be. I expect that tightening this up will improve the stability of our editor.

It's possible that some things like this are actually just API changes between major versions (though obviously also possible that we're just passing in invalid things now, too).

@jimrandomh jimrandomh marked this pull request as ready for review May 23, 2024 16:57
@jimrandomh jimrandomh requested a review from a team as a code owner May 23, 2024 16:57
@jimrandomh jimrandomh requested review from Will-Howard and removed request for a team May 23, 2024 16:57
@jimrandomh
Copy link
Collaborator Author

Looks like this broke Cypress tests somehow. It doesn't correspond to anything particularly broken on the client, and the classname it looks for to find CkEditor isntances is still used. Hrm. Going to try this with Playwright.

@jimrandomh
Copy link
Collaborator Author

Grumble, failure appears only in Cypress, but looks real-enough that it needs fixing.

@jpaddison3
Copy link
Collaborator

Try merging master and see it occurs on playwright

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants