ckeditor5 icon indicating copy to clipboard operation
ckeditor5 copied to clipboard

Error after running manual tests with DLLs if DLLs are built in development mode

Open psmyrek opened this issue 2 years ago • 2 comments

📝 Provide detailed reproduction steps (if any)

  1. Build DLLs in development mode: yarn run dll:build --dev.
  2. Run manual tests that require DLLs: yarn run manual -f ckeditor5.

✔️ Expected result

No error in the console.

❌ Actual result

Module parse failed: 'import' and 'export' may only appear at the top level (15204:20)
File was processed with these loaders:
 * ./node_modules/@ckeditor/ckeditor5-dev-tests/lib/utils/ck-debug-loader.js
You may need an additional loader to handle the result of these loaders.
|  */
|
> /* @if CK_DEBUG */  import { CKEditorError } from 'ckeditor5/src/utils';
|
| /**

❓ Possible solution

Probably this dynamic import causes this error log: https://github.com/ckeditor/ckeditor5/blob/38979ed46eb536b8064f48354e452499c6f71fd8/packages/ckeditor5-table/src/tablewalker.js#L10


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

psmyrek avatar Sep 16 '22 07:09 psmyrek

The following change helps:

diff --git a/packages/ckeditor5-table/src/tablewalker.js b/packages/ckeditor5-table/src/tablewalker.js
index 4cc18029c6..2b06695f6d 100644
--- a/packages/ckeditor5-table/src/tablewalker.js
+++ b/packages/ckeditor5-table/src/tablewalker.js
@@ -7,7 +7,8 @@
  * @module table/tablewalker
  */

-// @if CK_DEBUG // import { CKEditorError } from 'ckeditor5/src/utils';
+// eslint-disable-next-line no-unused-vars
+import { CKEditorError } from 'ckeditor5/src/utils';

 /**
  * The table iterator class. It allows to iterate over table cells. For each cell the iterator yields

However, it deserves a comment and perhaps, to avoid the same problems in the future, for a new eslint rule.

pomek avatar Sep 21 '22 08:09 pomek

This makes sense. With one exception: Where we import some dev utils this way.

TODOs:

  • Check whether tree-shaking would remove such dev utils from a package if they are imported but not used.
  • Add a comment why this is done this way.
  • Add an ESLint rule verifying this?

Reinmar avatar Sep 22 '22 09:09 Reinmar

Check whether tree-shaking would remove such dev utils from a package if they are imported but not used.

It works. I followed the steps below:

  • Update packages/ckeditor5-table/src/tablewalker.js with the following snippet:

    diff --git a/packages/ckeditor5-table/src/tablewalker.js b/packages/ckeditor5-table/src/tablewalker.js
    index 4cc18029c6..d3d58d0c13 100644
    --- a/packages/ckeditor5-table/src/tablewalker.js
    +++ b/packages/ckeditor5-table/src/tablewalker.js
    @@ -7,7 +7,12 @@
      * @module table/tablewalker
      */
    
    -// @if CK_DEBUG // import { CKEditorError } from 'ckeditor5/src/utils';
    +// eslint-disable-next-line no-unused-vars
    +import { CKEditorError } from 'ckeditor5/src/utils';
    +
    +function nonUsedAnywhere() {
    +	console.log( 'A function that is not used anywhere.' );
    +}
    
     /**
      * The table iterator class. It allows to iterate over table cells. For each cell the iterator yields
    
  • Prepare development DLL build for ckeditor5-table: yarn run dll:build --mode=development

  • Find the executable JS file (build/table.js). It should not be minified and should contain the changes we just added.

  • Prepare DLL build without the development mode: yarn run dll:build

  • The executable file does not contain changes related to the nonUsedAnywhere() function.

pomek avatar Oct 06 '22 05:10 pomek

☝️ The same scenario happens when I import an external module:

import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';

pomek avatar Oct 06 '22 05:10 pomek

Scope

  • The Module parse failed: 'import' and 'export' may only appear at the top level error will not occur if we use require() instead of import.
    • We already use require(): https://github.com/ckeditor/ckeditor5/blob/99be45bf13c2de73f272f3f687bd50ef2603406a/packages/ckeditor5-engine/src/controller/editingcontroller.ts#L40
  • Create a new rule for ESLint that uses require() instead of import in debug imports.
    • We already wrote a few of them.
  • Describe the new rule in the Code style guide.

pomek avatar Oct 12 '22 07:10 pomek