Closed pomek closed 4 months ago
Applying the proposed fix results in errors in the Table package. Let's try to define the property outside the class.
Here is a short description of why it exploded:
BalloonPanelView
defines a static property #defaultPositions
.BalloonPanelView
class is defined.
Related ~issue~release: https://github.com/evanw/esbuild/releases/tag/v0.18.4.
- Static properties aren't available. I am still trying to understand why it worked before. Perhaps it is related to esbuild itself.
I guess this is the cause (from the link you posted), this is what @niegowski mentioned as well in our discussion:
// Original code
export class Foo {
static foo = () => Foo
}
// Old output (with --bundle)
var _Foo = class {
};
var Foo = _Foo;
__publicField(Foo, "foo", () => _Foo);
// New output (with --bundle)
var Foo = class _Foo {
static foo = () => _Foo;
};
Also, we were trying to pass the --bundle
option somewhere in the esbuild-loader
. Unfortunately, it is not supported. esbuild#transform()
also does not accept it. The latter is used by the loader to produce the TS code.
We have identified more issues in the code base. There needs to be more than the proposed fix to make the CKEditor 5 compatible with the latest version of esbuild.
diff --git a/packages/ckeditor5-ui/src/view.ts b/packages/ckeditor5-ui/src/view.ts
index 43467aef0d..4be2f81948 100644
--- a/packages/ckeditor5-ui/src/view.ts
+++ b/packages/ckeditor5-ui/src/view.ts
@@ -159,7 +159,7 @@ export default class View<TElement extends HTMLElement = HTMLElement> extends Do
*/
public template?: Template;
- public viewUid?: string;
+ declare public viewUid?: string;
/**
* Collections registered with {@link #createCollection}.
diff --git a/packages/ckeditor5-ui/src/viewcollection.ts b/packages/ckeditor5-ui/src/viewcollection.ts
index 9ff8614d8f..47f4cf42c0 100644
--- a/packages/ckeditor5-ui/src/viewcollection.ts
+++ b/packages/ckeditor5-ui/src/viewcollection.ts
@@ -58,7 +58,7 @@ import type View from './view';
* of a {@link module:ui/template~Template template}.
*/
export default class ViewCollection<TView extends View = View> extends Collection<TView> {
- public id?: string;
+ declare public id?: string;
/**
* A parent element within which child views are rendered and managed in DOM.
Then, we had another issue. Unfortunately, we couldn't resolve it on a call. It requires deeper debugging and understanding of the differences between output produced by esbuild.
Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
Read more: https://ckeditor.com/docs/ckeditor5/latest/support/error-codes.html#error-Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
at Template._renderElementChildren (packages/ckeditor5-ui/src/template.ts:717:17 <- entry-point.1149032951.js:61417:23)
at Template._renderElement (packages/ckeditor5-ui/src/template.ts:458:8 <- entry-point.1149032951.js:61289:10)
at Template._renderNode (packages/ckeditor5-ui/src/template.ts:441:16 <- entry-point.1149032951.js:61275:19)
at Template.render (packages/ckeditor5-ui/src/template.ts:141:21 <- entry-point.1149032951.js:61009:23)
at ListItemView.render (packages/ckeditor5-ui/src/view.ts:494:33 <- entry-point.1149032951.js:64032:36)
at ListItemView.<anonymous> (packages/ckeditor5-utils/src/observablemixin.ts:277:33 <- entry-point.1149032951.js:68757:37)
at ListItemView.fire (packages/ckeditor5-utils/src/emittermixin.ts:241:31 <- entry-point.1149032951.js:67281:35)
at <computed> [as render] (packages/ckeditor5-utils/src/observablemixin.ts:281:17 <- entry-point.1149032951.js:68760:21)
at ViewCollection._renderViewIntoCollectionParent (packages/ckeditor5-ui/src/viewcollection.ts:211:9 <- entry-point.1149032951.js:64206:12)
at ViewCollection.<anonymous> (packages/ckeditor5-ui/src/viewcollection.ts:82:9 <- entry-point.1149032951.js:64097:12)
To reproduce the issue, you must force install the latest esbuild
package. It can be done via the resolutions
field in pkg.json or reverting this commit (https://github.com/ckeditor/ckeditor5-dev/commit/257c01758c73af99f68dcabaa06785bdb9d11f55) before installing deps.
The main difference here is that esbuild
stopped polyfilling class properties. In TSC we used a target
that would polyfill class properties (it assigns values to properties in the class constructor). This led to some mistakes in code that would not work if class properties were not polyfilled:
BalloonPanelView#defaultPositions
. Polyfilled code was initializing static properties one by one so the code worked.id in obj
) behaves differently for public id:string
vs. declare public id:string
. While not initialized in the constructor, the property would not be set on the class instance in polyfilled code, but in plain JS this property exists and in
check fails.Template
that was trying to handle ViewCollection
items but they were not rendered (we do not know yet why).As a test, I replaced all properties by adding declare
keyword and the manual test started working correctly (just a brief check). This could be the solution that reflects the TSC-generated code, no JS class properties defined, only assigned in the constructor. There is a tradeoff of overrides
keyword that is not allowed for declare
fields. I think we should find a better solution and maybe use declare
keyword only for properties that require it.
We spent some time refining this, it's a complex matter, that mostly touches how we run tests. Pinning esbuild-loader
is a good workaround right now, we will not prioritize this as critical right now.
One thing that we noticed in the esbuild playground, is that target=2019
produces a js that looks workable. Could we check again if we could pass this target in the esbuild-loader
? @psmyrek @pomek
Potentially, we could start fixing this when we decide to move to a different target, e.g. esnext
.
Passing target=es2019
in esbuild-loader
does not help. We checked it yesterday.
Passing
target=es2019
inesbuild-loader
does not help. We checked it yesterday.
Question if we shouldn't file an issue to esbuild-loader
if it produced JS that ignores this option (compare esbuild-loader
output vs. the esbuild
).
This issue can probably be solved with the Static initialization blocks introduced in ES2022
.
Example:
class Test {
static staticProperty1 = 'Property 1';
static staticProperty2;
static {
this.staticProperty2 = this.staticProperty1;
}
}
console.log(Test.staticProperty1); // Output: "Property 1"
console.log(Test.staticProperty2); // Output: "Property 1"
Bumping target
will probably be part of the new installation method initiative, so you can leave it to the core team.
This issue will be fixed in https://github.com/ckeditor/ckeditor5-dev/pull/951 and https://github.com/ckeditor/ckeditor5/pull/16318.
📝 Provide detailed reproduction steps (if any)
Make sure to install the latest
esbuild
(0.18.x
).Try to execute tests:
✔️ Expected result
❌ Actual result
❓ Possible solution