FIX: Fixed an issue with explicit value assignment due to babel trasp…

deniszografski commented

2023-03-16 13:46:06 +01:00

(Migrated from github.com)

@marijnh

This is a continuation of the conversation on this pull request

After a detailed investigation, the cause of the unexpected behavior was discovered if a babel transpiler was used in the stack.

Here is a problem demonstration with an example of code within the Selection class constructor with a visible property using Babel with plugin.

 `constructor(
    $anchor,
    $head, ranges) {
      _defineProperty(this, "ranges", void 0);
      _defineProperty(this, "visible", void 0); --> PROBLEM
      this.$anchor = $anchor;
      this.$head = $head;
      this.ranges = ranges || [new SelectionRange($anchor.min($head), $anchor.max($head))];
    }`

This definition of property within the constructor actually overrides the default true value and causes unexpected selection behavior in all parts of the application which is demonstrated in attachment within previous PR.

The problem was solved by explicitly setting the value (true) to the visible property in the Selection class.

`class Selection {
  visible = true
}`

@marijnh This is a continuation of the conversation on this [pull request](https://github.com/ProseMirror/prosemirror-view/pull/155) After a detailed investigation, the cause of the unexpected behavior was discovered if a babel transpiler was used in the stack. Here is a problem demonstration with an example of code within the `Selection` class constructor with a `visible` property using Babel with [plugin](https://babeljs.io/docs/babel-plugin-proposal-class-properties). ``` `constructor( $anchor, $head, ranges) { _defineProperty(this, "ranges", void 0); _defineProperty(this, "visible", void 0); --> PROBLEM this.$anchor = $anchor; this.$head = $head; this.ranges = ranges || [new SelectionRange($anchor.min($head), $anchor.max($head))]; }` ``` This definition of property within the constructor actually overrides the default `true` value and causes unexpected selection behavior in all parts of the application which is demonstrated in attachment within previous [PR](https://github.com/ProseMirror/prosemirror-view/pull/155). The problem was solved by explicitly setting the value (`true`) to the `visible` property in the Selection class. ``` `class Selection { visible = true }` ```

marijnh commented

2023-03-16 14:17:38 +01:00

(Migrated from github.com)

This is a bug in the plugin. As you can see on the TypeScript playground, TS itself doesn't emit anything for properties declared with a !. That one is intentionally put on the prototype, on instances, since it only needs to exist per class, and it'd be wasteful to store it on every instance.

So I suggest you report this with the project that generates that code incorrectly (or move to another package for this).

This is a bug in the plugin. As you can see on the [TypeScript playground](https://www.typescriptlang.org/play?target=1&ssl=3&ssc=1&pln=3&pc=2#code/MYGwhgzhAECC0G8BQ1XQA4CcD26CEAXNAEbbYgCmYAdkgL5A), TS itself doesn't emit anything for properties declared with a `!`. That one is intentionally put on the prototype, on instances, since it only needs to exist per class, and it'd be wasteful to store it on every instance. So I suggest you report this with the project that generates that code incorrectly (or move to another package for this).

deniszografski commented

2023-03-16 15:15:29 +01:00

(Migrated from github.com)

@marijnh

Sorry, I probably missed something.

For example, if we run this code below ( without transpiling ) in the browser, we will get undefined.

`<html>
  <script>
    class Foo {
      bar
    }

    Foo.prototype.bar = true;

    console.log(new Foo().bar);
  </script>
</html>`

Isn't that what a babel plugin does for us in this case?

The claim is that browser does the same as babel while TS does it wrong?

@marijnh Sorry, I probably missed something. For example, if we run this code below ( without transpiling ) in the browser, we will get `undefined`. ``` `<html> <script> class Foo { bar } Foo.prototype.bar = true; console.log(new Foo().bar); </script> </html>` ``` Isn't that what a babel plugin does for us in this case? **The claim is that browser does the same as babel while TS does it wrong?**

marijnh commented

2023-03-16 17:17:18 +01:00

(Migrated from github.com)

Sorry, I probably missed something.

Yes, the ! after the property definition in the ProseMirror sources. That's a TypeScript thing telling it that this property exists, but it shouldn't actually define/initialize it, just believe us on that.

> Sorry, I probably missed something. Yes, the `!` after the property definition in the ProseMirror sources. That's a TypeScript thing telling it that this property exists, but it shouldn't actually define/initialize it, just believe us on that.

👍 1

deniszografski commented

2023-03-16 18:35:04 +01:00

(Migrated from github.com)

Thank you @marijnh for the detailed explanation and all the best.

deniszografski commented

2023-03-17 13:45:31 +01:00

(Migrated from github.com)

@marijnh

After investing time in researching about this problem, I noticed that official typescript documentation has a configuration property.

I am aware that the integration of configuration flag is not trivial but
could you think about how to integrate this configuration flag into your projects?

Setting that flag would undoubtedly solve the potencial problems in lib.

@marijnh After investing time in researching about this problem, I noticed that official typescript documentation has a configuration [property](https://www.typescriptlang.org/tsconfig#useDefineForClassFields). I am aware that the integration of configuration flag is not trivial but could you think about how to integrate this configuration flag into your projects? Setting that flag would undoubtedly solve the potencial problems in lib.

marijnh commented

2023-03-17 19:34:57 +01:00

(Migrated from github.com)

Setting that flag would undoubtedly solve the potencial problems in lib.

How? As I said, I want this to be only on the prototype.

> Setting that flag would undoubtedly solve the potencial problems in lib. How? As I said, I _want_ this to be only on the prototype.

ilucin commented

2023-03-17 20:36:45 +01:00

(Migrated from github.com)

@marijnh can we declare the property instead? That would fix the issue where property value is being overriden in the constructor. We would achieve basically the same thing from TS perspective and the property would only exist on the prototype.

So my suggestion is to do this:

declare visible: boolean;

instead of:

visible!: boolean;

@marijnh can we `declare` the property instead? That would fix the issue where property value is being overriden in the constructor. We would achieve basically the same thing from TS perspective and the property would only exist on the prototype. So my suggestion is to do this: ``` declare visible: boolean; ``` instead of: ``` visible!: boolean; ```

marijnh commented

2023-03-20 11:46:51 +01:00

(Migrated from github.com)

I think a more fundamental question is why you are compiling the TypeScript yourself, rather than using the dist/ files included in the distributed package.

I think a more fundamental question is why you are compiling the TypeScript yourself, rather than using the `dist/` files included in the distributed package.

ilucin commented

2023-03-20 13:42:55 +01:00

(Migrated from github.com)

@marijnh The exclamation mark is non-null assertion operator. It is used for saying that the property "is not null", not that "it exists and is defined somwhere else". The declare property has better semantics for this scenario.

This doesn't have anything to do with the packaging/build system limitations in my project.

@marijnh The exclamation mark is non-null assertion operator. It is used for saying that the property "is not null", not that "it exists and is defined somwhere else". The `declare` property has better semantics for this scenario. This doesn't have anything to do with the packaging/build system limitations in my project.

marijnh commented

2023-03-20 14:01:59 +01:00

(Migrated from github.com)

The exclamation mark is non-null assertion operator

Not in this context.

This doesn't have anything to do with the packaging/build system limitations in my project.

This TypeScript code doesn't exist in the build output of the package, so if it is causing problems in your setup, that does suggest you're doing something odd.

> The exclamation mark is non-null assertion operator Not in this context. > This doesn't have anything to do with the packaging/build system limitations in my project. This TypeScript code doesn't exist in the build output of the package, so if it is causing problems in your setup, that does suggest you're doing something odd.

ilucin commented

2023-03-20 14:12:57 +01:00

(Migrated from github.com)

@marijnh I'm talking about language semantics here - it's being misused here. I've submitted a separate PR for this #22. It's also prevention of a future problem when useDefineForClassFields becomes the default behavior.

I can't see any reason not to merge this in? :)

Thanks

@marijnh I'm talking about language semantics here - it's being misused here. I've submitted a separate PR for this #22. It's also prevention of a future problem when `useDefineForClassFields` becomes the default behavior. I can't see any reason not to merge this in? :) Thanks

ilucin commented

2023-03-20 14:18:46 +01:00

(Migrated from github.com)

This TypeScript code doesn't exist in the build output of the package, so if it is causing problems in your setup, that does suggest you're doing something odd.

@marijnh You're right on that part - and I will make changes in my project to use the dist files. But don't you want your TS source to use proper semantics and to be more future-proof regarding this compiler flag?

> This TypeScript code doesn't exist in the build output of the package, so if it is causing problems in your setup, that does suggest you're doing something odd. @marijnh You're right on that part - and I will make changes in my project to use the dist files. But don't you want your TS source to use proper semantics and to be more future-proof regarding this compiler flag?

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

FIX: Fixed an issue with explicit value assignment due to babel trasp… #21

Pull request closed