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

Typescript compiler is removing some important comments. #13381

Closed
mremiszewski opened this issue Feb 2, 2023 · 5 comments · Fixed by #13477
Closed

Typescript compiler is removing some important comments. #13381

mremiszewski opened this issue Feb 2, 2023 · 5 comments · Fixed by #13477
Assignees
Labels
squad:devops Issue to be handled by the Devops team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@mremiszewski
Copy link
Contributor

mremiszewski commented Feb 2, 2023

📝 Provide detailed reproduction steps (if any)

Some code in *.ts files that is marked with CK_DEBUG is being removed by Typescript compiler because it's treated as a comment.

✔️ Expected result

No lines with CK_DEBUG are being removed during compilation.

❌ Actual result

Some lines with CK_DEBUG are being removed during compilation.

❓ Possible solution

Workaround:

/**
 * @license 
 */

/**
 * @module 
 */

// ✅ comment followed by an used import

import usedFunc from 'foo'

// ❌ comment followed by an unused import

import unusedFunc from 'bar'

// ❌ comment followed by a type import

import type SomeType from 'sometype'

// ✅ comment followed by a class

/**
 * 
 */
class A {

	// ✅ comment at the beginning of a class 

	constructor() {

	}

	// ✅ comment in the middle of a class 

	/**
	 * doc
	 */
	public method() {
		// ✅ comment inside a method 
		while (true) {
			usedFunc();
			// ✅ comment inside a loop 
		}
	}

	// ❌ comment at the end of a class 
}

// ❌ comment followed by an interface

/**
 * 
 */
interface B {

	/**
	 * doc
	 */
	readonly x: string;
	// ❌ comment inside interface

}

// ✅ comment followed by an export;

export { A };

// ❌ comment followed by a type export;

export type { A as A2 };

// ✅ comment at the end of file

Comments with ❌ would be deleted during compilation, and comments with ✅ would be preserved.
All lines in *.ts files with CK_DEBUG or CK_DEBUG_<something> should be investigated whether they are being removed during compilation and moved to a place where they wouldn't.
The same should be done for *.js files as the same issue will affect them when they are rewritten to Typescript.

Actual solution:
We should think about a different approach to code that should be available only in debug mode.

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

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

@mremiszewski mremiszewski added type:bug This issue reports a buggy (incorrect) behavior. squad:devops Issue to be handled by the Devops team. labels Feb 2, 2023
@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Feb 9, 2023
@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Feb 13, 2023
@przemyslaw-zan
Copy link
Member

przemyslaw-zan commented Feb 14, 2023

Solution that works is changing the order of ck-debug-loader and ts-loader in our webpack config. Thanks to this, CK_DEBUG lines which are needed, will be uncommented before being passed on to TS compiler, and thus preserved. I've confirmed this by looking at output generated by webpack in both loader orders, as well as checking whether this solution works in tests directly.

Some caveats:

  • We need to ensure that all of the CK_DEBUG comments inside TS files are valid TS code that will not throw an error during compilation.
  • Comments inside interfaces and other structures that only exist in TS will not be preserved.

Output generated by Webpack

  • ✅ - CK_DEBUG that is preserved before changes
  • ❌ - CK_DEBUG that is not preserved before changes

TS input file content

// ✅ comment followed by an used import

import usedFunc from './usedfunc';

// ❌ comment followed by an unused import

import unusedFunc from './unusedfunc';

// ❌ comment followed by a type import

import type { Type } from './type';

// @if CK_DEBUG // console.warn( '✅' );

/**
 *
 */
class A {
	// @if CK_DEBUG // value1 = '✅';

	constructor() {

	}

	// @if CK_DEBUG // value2 = '✅';

	/**
	 * doc
	 */
	public method(): void {
		// @if CK_DEBUG // console.warn( '✅' );
		while ( true ) {
			usedFunc();
			// @if CK_DEBUG // console.warn( '✅' );
		}
	}

	// @if CK_DEBUG // value3 = '❌';
}

// @if CK_DEBUG // console.warn( '❌' );

/**
 *
 */
interface B {

	/**
	 * doc
	 */
	readonly x: string;

	// @if CK_DEBUG // readonly y: '❌';

}

// @if CK_DEBUG // console.warn( '✅' );

export { A };

// @if CK_DEBUG // console.warn( '❌' );

export type { A as A2 };

// @if CK_DEBUG // console.warn( '✅' );

const foo = new A();

console.log( foo );

Webpack output currently:

this is invalid JS

// ✅ comment followed by an used import

/* @if CK_DEBUG */  console.warn( '✅' );
/**
 *
 */
class A {
    /* @if CK_DEBUG */  value1 = '✅';
    constructor() {
    }
    /* @if CK_DEBUG */  value2 = '✅';
    /**
     * doc
     */
    method() {
        /* @if CK_DEBUG */  console.warn( '✅' );
        while (true) {
            (0,_usedfunc__WEBPACK_IMPORTED_MODULE_0__["default"])();
            /* @if CK_DEBUG */  console.warn( '✅' );
        }
    }
}
/* @if CK_DEBUG */  console.warn( '✅' );

/* @if CK_DEBUG */  console.warn( '✅' );
const foo = new A();
console.log(foo);

Webpack output after switching loader order:

this is valid JS

// ✅ comment followed by an used import

/* @if CK_DEBUG */ console.warn('✅');
/**
 *
 */
class A {
    constructor() {
        /* @if CK_DEBUG */ this.value1 = '✅';
        /* @if CK_DEBUG */ this.value2 = '✅';
        /* @if CK_DEBUG */ this.value3 = '❌';
    }
    /**
     * doc
     */
    method() {
        /* @if CK_DEBUG */ console.warn('✅');
        while (true) {
            (0,_usedfunc__WEBPACK_IMPORTED_MODULE_0__["default"])();
            /* @if CK_DEBUG */ console.warn('✅');
        }
    }
}
/* @if CK_DEBUG */ console.warn('❌');
/* @if CK_DEBUG */ console.warn('✅');

/* @if CK_DEBUG */ console.warn('❌');
/* @if CK_DEBUG */ console.warn('✅');
const foo = new A();
console.log(foo);

@pomek
Copy link
Member

pomek commented Feb 15, 2023

Sounds good. Make sure to update both test environments: automated and manual.

@przemyslaw-zan
Copy link
Member

PRs: (they have to be used together, either one without the other will not work)

Despite there being ck-debug-loader in manual tests webpack config, there are no CK_DEBUG inside manual tests.

@przemyslaw-zan
Copy link
Member

@mremiszewski Were you you dealing with a specific case when describing this issue? If so, could you please check whether those PRs fix the issue?

pomek added a commit to ckeditor/ckeditor5-dev that referenced this issue Feb 17, 2023
Fix (tests): Swapped an order of loaders when processing the TypeScript files. The `ck-debug-loader` is executed before `ts-loader` to avoid the removal of `CK_DEBUG_*` comments from the source code. See ckeditor/ckeditor5#13381.

MAJOR BREAKING CHANGE (tests): The code specified in the `CK_DEBUG_*` flags must be valid TypeScript code as it is processed before `ts-loader`.
pomek added a commit that referenced this issue Feb 20, 2023
Fixed the TypeScript code hidden behind the `CK_DEBUG_*` flags. Closes #13381.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Feb 20, 2023
@CKEditorBot CKEditorBot added this to the iteration 61 milestone Feb 20, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:devops Issue to be handled by the Devops team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants