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

Added the esbuild-loader for JavaScript files to speed up rebuilding the manual and automated tests #770

Closed
wants to merge 1 commit into from

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented May 27, 2022

Suggested merge commit message (convention)

Other (tests): Added the esbuild-loader for JavaScript files to speed up rebuilding the manual and automated tests.


❗Please read the description below before trying to merge this PR.

TL;DR: In my opinion it is not worth it, as the profit is no or negligible. Probably moving fully to the esbuild from webpack would accelerate all things, but this is another story (for another issue).

Let's start with the raw results. The table below shows the average time needed to compile the tests. Each result is the average value calculated of 5 runs.

 Average compilation time
Manual testsAutomated tests
beforeafterbeforeafter
all packagescrashed11 min 23 sec12.5 sec12.4 sec
ckeditor5-engine11.8 sec8.5 sec4.9 sec4.9 sec
ckeditor5-image9.7 sec8.1 sec4.8 sec4.9 sec
ckeditor5-list7.1 sec6.9 sec4.9 sec4.8 sec
ckeditor5-alignment5.0 sec5.3 sec3.8 sec3.7 sec

1 There is a workaround to prevent it from crashing by running yarn run manual --disable-watch.

Manual tests

In manual tests, there is a benefit in using esbuild-loader only for packages that contain a lot of tests. For the ckeditor5-engine package it is about 30% and for the ckeditor5-image - around 15%. For other packages where the are less tests, no measurable difference can be noticed. Before and after the change, the compilation time can be assumed as the same.

One unexpected advantage of the esbuild-loader in manual tests is that we can suddenly build all of your manual tests at once. Previously, without the new loader, it is not possible and the yarn run manual script crashes and burns in an epic manner. On the other hand, we are still also able to build all manual tests in the old way with yarn run manual --disable-watch, which means that files are not watched and the source maps are not generated, but who would like the active watcher and source maps when running all manual tests?

The big downside of using this esbuild-loader is that it removes all comments from source maps and it can't be turned off. This is not acceptable.

In conclusion, despite some (but not significant) acceleration in large packages, I don't see any point in integrating the esbuild-loader into manual test tooling.

Automated tests

In automated tests, the results before and after the changes are practically the same so I also don't see any point in integrating the esbuild-loader into the tooling.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.614% when pulling 04923aa on ci/1618 into 74f7cda on master.

@psmyrek psmyrek requested a review from pomek May 27, 2022 10:35
@pomek
Copy link
Member

pomek commented May 30, 2022

We decided to abandon adding the esbuild-loader into the CKEditor 5 environment.

@pomek pomek closed this May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants