search

ckeditor / ckeditor5-react

Official CKEditor 5 React component.
https://ckeditor.com/ckeditor-5
Other
425 stars 99 forks source link

Migrate to ESM #490

Closed filipsobol closed 4 months ago

filipsobol commented 4 months ago

Suggested merge commit message (convention)

Internal: Migrate from webpack and Karma to Vite and Vitest. See ckeditor/ckeditor5#16616.

Internal: Fix test coverage which only covered a small portion of the codebase.

MINOR BREAKING CHANGE: Migrate to ESM. Related to ckeditor/ckeditor5#16616.

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

filipsobol commented 4 months ago

Failing tests are related to our migration from Enzyme to React Testing Library. They will be fixed in other PR and merged into this branch.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 27f11a66-2383-4b8c-a47d-16b6e2c5dd4b

Details

Totals Coverage Status
Change from base Build f69e3b88-1a21-4a49-b767-e752203d3308: 0.0%
Covered Lines: 539
Relevant Lines: 539
💛 - Coveralls
martnpaneq commented 4 months ago

@filipsobol @pomek That's a very nice cleanup of the repo and I like that you managed to get rid of demos directories. Please have a look at the minor comments that I left and this should be ready to merge :)

Reinmar commented 4 months ago

Why are we removing all the demos? Are we sure they didn't serve some purpose?

At least one of them is frequently visited. 

Mati365 commented 4 months ago

@Reinmar demos are still there

Reinmar commented 4 months ago

@Reinmar demos are still there

Where?

:point_up: this was a an honest question, but I realized that since you asked they are probably somewhere...

I would never think to look for them in src/demos/ :open_mouth: And I don't get the purpose of ./index.html. Who and how is supposed to open that file in the browser? Is this a common pattern among React libraries?

Mati365 commented 4 months ago

@Reinmar I get your point. Yep, src/ placement might be misleading. It's not the biggest problem. I'm sure that there will be accidential imports between manuał/ and production code which we don't want for sure.

Reinmar commented 4 months ago

I talked about this with @filipsobol too.

We also saw the need to update https://github.com/ckeditor/ckeditor5-react/tree/master?tab=readme-ov-file#contributing

filipsobol commented 4 months ago

TODO: Check Vite target

filipsobol commented 4 months ago

I addresses all comments and suggestions. The demos were moved from /src/demos/* to /demos/* so they are more visible.

Mati365 commented 4 months ago

fyi, I discovered one issue related to incorrect detection of Context in useMultiRootEditor hook. Long story short - our enzyme tests were not testing context forwarding at all, and it was possible to use the wrong context while having the test suite succeed. That was introduced in React 19 integration fixes, and it was not released. It's not a huge issue, and it was much easier to do that in the scope of this PR due to lack of proper enzyme testing. I added proper RTL testing.

https://github.com/ckeditor/ckeditor5-react/pull/490/files#diff-a495913bafb3d316e8cc54a39c3de3a4f04b1746ee0f40a0e5dd722d0a955533R368-R375

filipsobol commented 4 months ago

I can see that you changed the convention of file names in tests. Why did you make that change? Did you document this decision in any logbook?

This is an easy way to find the files with tests and ignore test utils - 'tests/**/*.test.[j|t]sx'. If you don't like these changes, I can revert them and just add a proper exclude in Vitest config.

filipsobol commented 4 months ago

As discussed, I added the UMD build for better backward compatibility, following the configuration recommendations from the Vite documentation: https://vitejs.dev/guide/build#library-mode.