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

Migrate CKEditor 5 to TypeScript #11704

Closed
Reinmar opened this issue May 4, 2022 · 62 comments
Closed

Migrate CKEditor 5 to TypeScript #11704

Reinmar opened this issue May 4, 2022 · 62 comments
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. domain:ts Epic squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented May 4, 2022

Back in 2015 when we were bootstrapping CKEditor 5 TypeScript wasn't yet mature enough and didn't yet seem as a safe choice taken the long horizon of CKEditor 5's life and fate of e.g. CoffeeScript or Backbone (that were the popular choices at that time).

Nowadays, for a large scale, complex project such as CKEditor 5, TypeScript is a clear choice. We've been actually looking at it for a longer time but it was clear that we need to pick the right moment for a migration. The moment has come 🥳

The main goals of the migration:

  • Improving the DX for the community by providing typings for CKEditor 5. This is by far the most popular request on our issue tracker and it's visible how important it is to the community also by the amount of work put by many people, including heroes such as @fedemp. Providing official typings will benefit integrators and plugin developers. It will improve the DX when working with CKEditor 5 and stability of the solutions.
  • Improving the core team experience and toolset. While using TypeScript will naturally make us happy (we really wanted to migrate for a long time 😄), we believe that this will have a positive outcome for everyone. CKEditor 5 is a complex project with a large API. We're rigorously documenting the types from day one, but we had to resort to JSDoc for that. It helps during development, but it's nowhere near what we'll have with TypeScript. Type validation, better API documentation, better suggestions, refactoring options – this will all speed up the development and potentially improve the quality even more.

We're right now cooking a plan of action, so I won't share more details for now. Some two last points that I want to mention for now are:

  • We're determined to not break the backward compatibility (except minor aspects). We are aiming at making the migration for a great majority of JS-based projects that used CKEditor 5 for now seamless.
  • This project will take several months due to the scale of CKEditor 5. We'll be migrating step by step, most likely starting from the infrastructure and one or two first packages.
  • We'll use this ticket for further developer as Typings for TypeScript #504 was already too long and does not exactly cover the entire scope of the project for us. We'll start sharing our ideas and questions on what should we deliver here in this thread and potentially in Typings for TypeScript #504 for better visibility too. But this one is the one.
  • Once again I'd like to thank @fedemp and everyone else who contributed to the community-driven DefinitelyTyped. For at least a couple of months it's still the place to go when you need typings for CKEditor 5.

Keep your fingers crossed and stay tuned for more information.

@Reinmar Reinmar added type:task This issue reports a chore (non-production change) and other types of "todos". squad:core Issue to be handled by the Core team. labels May 4, 2022
@Reinmar Reinmar added domain:dx This issue reports a developer experience problem or possible improvement. domain:ts labels May 5, 2022
@Reinmar Reinmar added the Epic label May 5, 2022
@arkflpc
Copy link
Contributor

arkflpc commented May 5, 2022

  • Once again I'd like to thank @fedemp and everyone else who contributed to the community-driven DefinitelyTyped. For at least a couple of months it's still the place to go when you need typings for CKEditor 5.

I don't know if it's possible for us to stay compatible with that, though. Should we?

@fedemp
Copy link
Contributor

fedemp commented May 5, 2022

  • Once again I'd like to thank @fedemp and everyone else who contributed to the community-driven DefinitelyTyped. For at least a couple of months it's still the place to go when you need typings for CKEditor 5.

I don't know if it's possible for us to stay compatible with that, though. Should we?

Official typings do not need to be compatible with DT typings. And if official typings somehow contradict those at DT, the official should be the definitive answer (assuming CKEditor is going to be rewritten in TS).

@fedemp
Copy link
Contributor

fedemp commented May 5, 2022

This project will take several months due to the scale of CKEditor 5. We'll be migrating step by step, most likely starting from the infrastructure and one or two first packages.

You team surely know better, but I suggest you start with utils, core, and lastly engine. utils have very little dependencies, and engine is a huge monster to tackle.

@Reinmar
Copy link
Member Author

Reinmar commented May 5, 2022

Hi @fedemp! I tried to reach out to you on LinkedIn. Could you check your invitations or write back to me at [email protected]?

@arkflpc
Copy link
Contributor

arkflpc commented Jun 8, 2022

Some ideas for post-MVP:

  • Better integration with Angular and React
  • Update guides

@arkflpc
Copy link
Contributor

arkflpc commented Jul 5, 2022

Status Update:

What we've done:

  1. We rewrote the ckeditor5-utils package to TypeScript (Rewrite ckeditor5-utils to TypeScript #11755).
  2. We're now able to run both manual and automated tests ([TS] Allow running tests written in TS (automated, manual) #11888).
  3. We have working rules for eslint ([TS MVP] Update ESLint configuration #11719).
  4. We can build DLLs (almost done, [TS MVP] Update DLL build process #11718).

It's all on a feature branch yet. But as soon as we'll finish updating the release process (#11720, work-in-progress), we can get to master branch and release it. The packages on NPM won't contain typings yet, because that would conflict with the community-provided ones. We're going to start generating them after we'll finish the work on ckeditor5-engine (#11724, work-in-progress), ckeditor5-ui (#11726) and ckeditor5-core (#11727) and stabilize the typings.

@fedemp
Copy link
Contributor

fedemp commented Jul 7, 2022

The packages on NPM won't contain typings yet, because that would conflict with the community-provided ones.

When the time comes, open an issue on DefinitelyTyped to remove those. 👍

@arkflpc
Copy link
Contributor

arkflpc commented Jul 8, 2022

We will, @fedemp. We're going to stabilize them first.

@Reinmar
Copy link
Member Author

Reinmar commented Aug 3, 2022

It's worth mentioning that CKEditor 5 v35.0.0 is out and it's the first version where TS code was used (namely @ckeditor/ckeditor5-utils) 👏

As for what's next...

We're right now reviewing #12188 and engine was by far the biggest and most complex package to port.

Next packages to migrate: core and utils. Soon, we'll be able to start porting features which should be easy to do concurrently and much much faster.

@arkflpc
Copy link
Contributor

arkflpc commented Sep 5, 2022

Status update

We released ckeditor5-engine (#11724) in TypeScript. ckeditor5-ui (#11726) and ckeditor5-core (#11727) are done on feature branch (ck/11726-11727-ts-core-ui). They will be merged in few next weeks.

The next milestone is to select API documentation generator and integrate it with our tooling. After it's done, we will go on with other packages that are in JavaScript yet.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 10, 2023

@Inviz Unfortunately, this is not the supported way to use the editor, and we have decided not to export the MutationObserver and commands from the main entry points. You can still access them as before, but you'll have to deal with potential TypeScript errors yourself.

A bit of context here. We decided to treat the index.ts entry points as sort of public API of packages. That will allow us to hide some implementation details of each package or things that we expect may change, while still allowing hacking the editor or its features via a direct access to each package's src/ (but at your own risk).

@Inviz
Copy link
Contributor

Inviz commented Mar 10, 2023

yeah i think i can deal with that. i'm hacking it already, so i can hack the types too.

@pomek
Copy link
Member

pomek commented Mar 13, 2023

The new alpha release is out: 👉 https://github.com/ckeditor/ckeditor5/releases/tag/v37.0.0-alpha.1.

@Inviz
Copy link
Contributor

Inviz commented Mar 14, 2023

There's another issue that is just quasy rellated to this:

When using ckeditor from sources without a build, vitest (being esm-first project) complains about CK5 modules not being esm modules, although they seem to should be in its opinion.

Module /Users/sitecore/Sites/feaas-components/ckeditor5/node_modules/@ckeditor/ckeditor5-utils/src/dom/findclosestscrollableancestor.js:14 
seems to be an ES Module but shipped in a CommonJS package. 
You might want to create an issue to the package "@ckeditor/ckeditor5-utils" 
asking them to ship the file in .mjs extension or add "type": "module" in their package.json.

Is this something that can be resolved?

@pomek
Copy link
Member

pomek commented Mar 14, 2023

Adding type: module into package.json would resolve the issue.

I'm wondering if sources should be marked as ES Module too.

I extracted the case to a separate issue: #13673.

@filipsobol
Copy link
Member

We have some great news for Vue developers! We just released an alpha version of our Vue integration that includes improved TypeScript support. We hope you'll give it a try and share your feedback to help us make sure it works well before we release a stable version. To install it, run npm install @ckeditor/ckeditor5-vue@alpha.

We hope to have similar news for the React and Angular folks next week.

@filipsobol
Copy link
Member

@Inviz I looked into the issues you reported in this comment. I hope my answers below will help.


Breaking: selectable in model writer is set to Node type, but according to docs it's https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_model_selection-Selectable.html this. So it doesnt accept position at this time.

The createSelection in model writer has two signatures (thanks to the “function overloading”). The first one expects a Node followed by PlaceOrOffset, and optional options. The other expects any Selectable except Node and optional options.

You can switch between the two definitions in your IDE using this switch:


Breaking: Plugin doesnt have .set method (for observable props)... it used to work

Please check if you have installed the alpha version of the both core and utils packages. If you have, and it doesn't work as shown in the example below (or like in this playground), please let us know.


EventInfo signature seems to have been simplified from EventInfo<doc, string> to EventInfo<string>

We don't try to maintain compatibility with DefinitelyTyped typings, so there may be cases like this here and there. The current signature is EventInfo<TName extends string = string, TReturn = unknown>. The first generic defines the type of the event name, and the other defines its return type.


I could not make assignment to Editor.builtinPlugins to work: Type 'typeof import("/Users/sitecore/Sites/feaas-components/ckeditor5/node_modules/@ckeditor/ckeditor5-link/src/autolink")' is not assignable to type 'PluginConstructor<Editor>'

I remember seeing this error before we migrated all plugins to TypeScript. Please check if the link plugin and any other plugins you have installed are also in the alpha version. If they are and you still see the error, please let us know.

I also recommend using override to set builtinPlugins or defaultConfig. The "old" way should still work fine, but the one shown below works better with TypeScript.

// Before
class MyEditor extends ClassicEditor {}
MyEditor.builtinPlugins = [];
MyEditor.defaultConfig = {}

// Now
class MyEditor extends ClassicEditor {
  public static override builtinPlugins = [];
  public static override defaultConfig = {};
}

We switched to the second way of defining built-in plugins because it produces better definitions (.d.ts), where the first approach was ignored.

 


I dont use config often, but i'm getting error on assigning heading. I think this is what Reinmar meant talking about module augmentation?

This is already fixed in the latest alpha.

@Inviz
Copy link
Contributor

Inviz commented Mar 21, 2023

@filipsobol Thanks Filip! I can confirm that using your suggestions i managed to fix all the issues I've had. Great job

@Inviz
Copy link
Contributor

Inviz commented Mar 22, 2023

@filipsobol Can it be somehow improved that we can jump from types to the code implementation? I think it would be hugely useful at least to jump to compiled js (if not ts). Right now i can only jump to the type it seems in vs code

@filipsobol
Copy link
Member

@Inviz This is the default behavior of IDEs for libraries that ship definition files, so it's not something we can do at the library level.

I don't know about other IDEs, but in VS Code you can right click on the element you want to jump to and select "Go to source definition".

@Inviz
Copy link
Contributor

Inviz commented Mar 22, 2023 via email

@Inviz
Copy link
Contributor

Inviz commented Mar 28, 2023

In Alpha.1 i could not import SelectionObserver from the engine. I guess it could be one of those things that i shouldnt do... it may be so.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 28, 2023

In Alpha.1 i could not import SelectionObserver from the engine. I guess it could be one of those things that i shouldnt do... it may be so.

Why do you need it? It's registered by default. We'll be trying to understand whether there are some cases we don't see ourselves but that are valid so we'll be asking every time :)

@filipsobol
Copy link
Member

Today we released an alpha version of our React integration that includes improved TypeScript support. We hope you'll give it a try and share your feedback to help us make sure it works well before we release a stable version. To install it, run npm install @ckeditor/ckeditor5-react@alpha.

@filipsobol
Copy link
Member

Last but not least, the Angular integration has also been updated and released as an alpha for testing. To install it, run npm install @ckeditor/ckeditor5-angular@alpha.

@arkflpc arkflpc added this to the iteration 61 (v37.0.0) milestone Apr 6, 2023
@arkflpc
Copy link
Contributor

arkflpc commented Apr 6, 2023

Hi everyone,

I am happy to announce that we have completed the migration of CKEditor5 project to TypeScript! We appreciate all the feedback that we received from the community during this process.

For those who are interested, we have documented the migration steps in our documentation (here). We have also created a separate documentation on how to use CKEditor5 in TypeScript projects (here).

We would like to invite everyone to provide their feedback in our other ticket (#12027) where we will track our future improvements.

Moving forward, we plan to update CKEditor5 Online Builder and ckeditor5-package-generator to use TypeScript, make typings work with DLLs, migrate the inspector to TS, update our Contribution Guide and other documentation to reflect that it is now a TypeScript project, and other miscellaneous stuff.

Thank you all for your continued support and contributions.

@arkflpc arkflpc closed this as completed Apr 6, 2023
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Apr 6, 2023
@Inviz
Copy link
Contributor

Inviz commented May 1, 2023

Event-based dispatcher dont seem to be typed? e.g. upcastDispatcher.on('element:a', (evt, data, conversionApi) => {} args dont have inferred types, as on is generic function

@mmichaelis
Copy link

Event-based dispatcher dont seem to be typed? e.g. upcastDispatcher.on('element:a', (evt, data, conversionApi) => {} args dont have inferred types, as on is generic function

Did you use generic types, as can be found in many example usages in the CKEditor 5 workspace? Such as here:

editor.data.upcastDispatcher.on<UpcastElementEvent>( 'element:h1', dataViewModelH1Insertion, { priority: 'high' } );
editor.data.upcastDispatcher.on<UpcastElementEvent>( 'element:h2', dataViewModelH1Insertion, { priority: 'high' } );
editor.data.upcastDispatcher.on<UpcastElementEvent>( 'element:h3', dataViewModelH1Insertion, { priority: 'high' } );

If not providing a generic argument, you will only have options of the BaseEvent:

public on<TEvent extends BaseEvent>(
event: TEvent[ 'name' ],
callback: GetCallback<TEvent>,
options?: CallbackOptions
): void {
this.listenTo( this, event, callback, options );
}

@Inviz
Copy link
Contributor

Inviz commented May 2, 2023

thanks. Is there a necessity to provide those types explicitly? It seems that typescript could infer by event name?

@Inviz
Copy link
Contributor

Inviz commented May 2, 2023

@Inviz This is the default behavior of IDEs for libraries that ship definition files, so it's not something we can do at the library level.

I don't know about other IDEs, but in VS Code you can right click on the element you want to jump to and select "Go to source definition".

I'd like to bring this up again. Right now if you "Go to source definition" you jump into JS file. Within that file you can jump between functions as well. but any function that is out of that file is any. So i can't navigate the codebase this way - only one way deep. Can this be improved?

@Witoso
Copy link
Member

Witoso commented May 4, 2023

@Inviz could you move this case to the #12027 as this ticket is closed?

@abduraufsherkulov
Copy link

Is there any demo how to upgrade from builder version of ckeditor to typescript? @arkflpc

@arkflpc
Copy link
Contributor

arkflpc commented May 15, 2023

Hi @abduraufsherkulov.

Custom builds produced by the online builder and DLL versions of packages provided by CKEditor 5 do not provide built-in typings yet. We plan to provide the support for those.

However, it is possible right now (although not straightforward): see this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. domain:ts Epic squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests