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

deniszografski · 2023-03-16T12:46:06Z

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
}`

…iler compatiblity Signed-off-by: DenisZ <[email protected]>

marijnh · 2023-03-16T13:17:38Z

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).

deniszografski · 2023-03-16T14:15:29Z

@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 · 2023-03-16T16:17:18Z

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.

deniszografski · 2023-03-16T17:35:04Z

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

deniszografski · 2023-03-17T12:45:31Z

@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 · 2023-03-17T18:34:57Z

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 · 2023-03-17T19:36:45Z

@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 · 2023-03-20T10:46:51Z

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 · 2023-03-20T12:42:55Z

@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 · 2023-03-20T13:01:59Z

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 · 2023-03-20T13:12:57Z

@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 · 2023-03-20T13:18:46Z

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?

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

af5ecfc

…iler compatiblity Signed-off-by: DenisZ <[email protected]>

marijnh closed this Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

deniszografski commented Mar 16, 2023 •

edited

Loading

marijnh commented Mar 16, 2023

deniszografski commented Mar 16, 2023

marijnh commented Mar 16, 2023

deniszografski commented Mar 16, 2023

deniszografski commented Mar 17, 2023

marijnh commented Mar 17, 2023

ilucin commented Mar 17, 2023 •

edited

Loading

marijnh commented Mar 20, 2023

ilucin commented Mar 20, 2023

marijnh commented Mar 20, 2023

ilucin commented Mar 20, 2023

ilucin commented Mar 20, 2023

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

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

Conversation

deniszografski commented Mar 16, 2023 • edited Loading

marijnh commented Mar 16, 2023

deniszografski commented Mar 16, 2023

marijnh commented Mar 16, 2023

deniszografski commented Mar 16, 2023

deniszografski commented Mar 17, 2023

marijnh commented Mar 17, 2023

ilucin commented Mar 17, 2023 • edited Loading

marijnh commented Mar 20, 2023

ilucin commented Mar 20, 2023

marijnh commented Mar 20, 2023

ilucin commented Mar 20, 2023

ilucin commented Mar 20, 2023

deniszografski commented Mar 16, 2023 •

edited

Loading

ilucin commented Mar 17, 2023 •

edited

Loading