github / catalyst

Catalyst is a set of patterns and techniques for developing components within a complex application.
https://github.github.io/catalyst/
MIT License
1.32k stars 49 forks source link

@target returns undefined using Vite #307

Closed robin4a4 closed 1 year ago

robin4a4 commented 1 year ago

Hi ! I am experiencing some issues with a simple example using vite. When I use @target it returns undefined as if the Object.defineProperty wasn't working properly. However, it works well without a decorator. I used the example provided here in the docs and added a button to trigger the action.

I created a minimal reproduction here, let me know if you prefer a github repo. We can see that output1 does not exists but the second one is here.

I believe this is fixed in the upcoming V2 but it appears that the V2 will be experiencing some delay because of the new decorator standards added to ECMAScript so I wanted to make it work with the stable release as we aim to use it in production.

Thanks in advance for any help and let me know if I can provide more details or context.

keithamus commented 1 year ago

I think I see the problem. In the compiled output the class looks like this:

export let HelloWorldElement = class extends HTMLElement {
  output1;
  get output2() {
    return findTarget(this, "output2");
  }
  greet() {
    if (this.output1)
      this.output1.textContent = `Hello, output 1!`;
    if (this.output2)
      this.output2.textContent = `Hello, output 2!`;
  }
};

The line output1; is causing the issue here. Catalyst defines the property on the prototype element, but output1; is defining a class field - which if left untranspiled will call Object.defineProperty(this, 'output1', {...}) during the constructor phase.

This is a bug with Catalyst that should be fixed - but it won't be especially easy, sadly. We could check for defined properties using getOwnPropertyDescriptors(). The problem will be that class fields always get run as the last stage of the constructor, which means we might have to defer to connectedCallback() instead... which isn't great.

One solution for now, if you're using typescript, is to use declare which elides the class field from output:

import { controller, target, findTarget } from '@github/catalyst';

@controller
export class HelloWorldElement extends HTMLElement {
-  @target output1: HTMLElement;
+  @target declare output1: HTMLElement;
  get output2() {
    return findTarget(this, 'output2') as HTMLElement;
  }
  greet() {
    if (this.output1) this.output1.textContent = `Hello, output 1!`;
    if (this.output2) this.output2.textContent = `Hello, output 2!`;
  }
}

Another solution is to transpile class field syntax, which avoids the need for declare.

You are right about 2.0 - it will break away from non-standard decorators and use the Stage 3 Decorators proposal which allows for field initializers, which would fix this problem... but that'll be a while off.

robin4a4 commented 1 year ago

I will explore both options but I can tell that using declare is indeed working. Thank you for your answer i think we can close !