fix(angular): 2-way data-binding for inlined Inputs

[alexandertrefz]

Pull request checklist

Please check if your PR fulfills the following requirements:

• [x] Tests for the changes have been added (for bug fixes / features)
• [x] Docs have been reviewed and added / updated if needed (for bug fixes / features)
• [x] Build (npm run build) was run locally for affected output targets
• [x] Tests (npm test) were run locally and passed
• [x] Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

• [x] Bugfix

What is the current behavior?

When using the new inlineProperties configuration option, properties only update correctly when they are used unbounded(<my-component prop="value" />), on the angular side. Bound(<my-component [prop]="componentProperty" />) properties are not synced correctly.

What is the new behavior?

Properties work correctly, no matter if they are used bound or unbounded.

Does this introduce a breaking change?

• [x] No

General Information

PR #497 by @kristilw introduced a long awaited fix for Angular, allowing the Angular Language Server to pick up the types of inputs correctly.

The change works correctly on the Typescript side, as well as at runtime, if the input properties are unbound. However, if they are bound the Angular Compiler does something that prevents the binding to work at all (at runtime), even though the Decorator establishing the getter and setter to pass the value along to the Web Component underneath is run correctly.

This is caused (for some reason) by the property being declared on the class itself, after compilation, like this:

export let MyComponent = class MyComponent {
    z;
    el;
    prop; // <-- THIS IS THE PROBLEM
    constructor(c, r, z) {
        this.z = z;
        c.detach();
        this.el = r.nativeElement;
    }
    static ɵfac = function MyComponent_Factory(t) { return new (t || MyComponent)(i0.ɵɵdirectiveInject(i0.ChangeDetectorRef), i0.ɵɵdirectiveInject(i0.ElementRef), i0.ɵɵdirectiveInject(i0.NgZone)); };
    static ɵcmp = /*@__PURE__*/ i0.ɵɵdefineComponent({ type: MyComponent, selectors: [["my-component"]], inputs: { prop: "prop" }, ngContentSelectors: _c0, decls: 1, vars: 0, template: function MyComponent_Template(rf, ctx) { if (rf & 1) {
            i0.ɵɵprojectionDef();
            i0.ɵɵprojection(0);
        } }, encapsulation: 2, changeDetection: 0 });
};

However, in order for typescript to create the type in such a way that the (badly implemented) Angular Language Service can pick up the types correctly, we need to declare them on the class.

The solution is to inline the getter and setter for the inputs directly, allowing Angular to pick up the type correctly, and the internal binding to the web component to work correctly as well.

This PR also changes the type of the el property, so that it reflects the accurate Web Component type, instead of the generic HTMLElement.

[kristilw]

Hm, for some reason I'm not able to reproduce the bug, but it might be because I'm using the outputType: 'standalone' and type: 'dist-custom-elements'. What is your configuration?

Not doubting that the issue is real, just curiosis why the error is occuring.

Thanks for helping to resolve bugs :)

[alexandertrefz]

That would explain why you didn't catch this one, I am using outputType: "component", and type: "dist", currently.

[dgonzalezr]

Unless I'm missing something, binding property value works for me (with Angular Module):

<bq-progress
  enable-tooltip
  thickness="large"
  [value]="progressValue"
></bq-progress>

export class AppComponent implements AfterViewInit {
  progressValue = 20;

  ngAfterViewInit() {
    this.updateProgress();
  }

  updateProgress() {
    const intervalId = setInterval(() => {
      this.progressValue++;
      if (this.progressValue > 100) {
        clearInterval(intervalId);
      }
    }, 250);
  }
}

I'm using outputType: 'component', this is the angular output target config:

angular({
  componentCorePackage,
  outputType: 'component', // Generate many component wrappers tied to a single Angular module (lazy/hydrated approach)
  directivesProxyFile: resolve(__dirname, '../beeq-angular/src/directives/components.ts').replace(/\\/g, '/'),
  directivesArrayFile: resolve(__dirname, '../beeq-angular/src/directives/index.ts').replace(/\\/g, '/'),
  valueAccessorConfigs: angularValueAccessorBindings,
  customElementsDir: 'dist/components'
}),

Here 👉🏼 is a code sandbox with the code example but I'm really curious about this and if there is a way to see/get a reproduction case. As mentioned before, maybe I'm missing something or the issue is related to something else different from what I've mentioned above (and if that's the case, my apologies 🙂 ).

[alexandertrefz]

@dgonzalezr This bug is specific to the new experimental inlineProperties configuration option, for the output, so in your case, update the output target config like this:

angular({
  componentCorePackage,
  outputType: 'component', // Generate many component wrappers tied to a single Angular module (lazy/hydrated approach)
  directivesProxyFile: resolve(__dirname, '../beeq-angular/src/directives/components.ts').replace(/\\/g, '/'),
  directivesArrayFile: resolve(__dirname, '../beeq-angular/src/directives/index.ts').replace(/\\/g, '/'),
  valueAccessorConfigs: angularValueAccessorBindings,
  customElementsDir: 'dist/components',
  inlineProperties: true
})

[dgonzalezr]

@dgonzalezr This bug is specific to the new experimental inlineProperties configuration option, for the output, so in your case, update the output target config like this:

Thank you @alexandertrefz for clarifying it 🙌🏼 . After testing the new experimental inlineProperties config option, I can confirm the issue.

[kristilw]

Looking at it more I found that it is unrelated to outputType: 'standalone' and type: 'dist-custom-elements', but probably the tsconfig of the Angular build. In the example angular project, the inlined props are removed from the final js build. @alexandertrefz, would you care to try the tsconfig as it is set up in the example project (link)?

If using getters and setters resolves the problem that could be a solution, but it would be even better if they are never included in the final output (leaving the generated js the same regardless of the value of inlineProperties).

[kristilw]

Playing around with the target setting in typescript, I found that by setting the compilerOptions.target: 'es2022' recreates the issue. However, the issue can then be resovled by setting another property in the config: compilerOptions.useDefineForClassFields: false. Care to test?

Not saying that this is a perfect solution, but just to verify

[dgonzalezr]

Playing around with the target setting in typescript, I found that by setting the compilerOptions.target: 'es2022' recreates the issue. However, the issue can then be resovled by setting another property in the config: compilerOptions.useDefineForClassFields: false. Care to test?

Not saying that this is a perfect solution, but just to verify

@kristilw I have applied the following changes based in your comment, and indeed worked for me with the new experimental config 🤷🏼‍♂️

I have no doubt that @alexandertrefz solutions could be the right approach, I'm just leaving some feedback so that all cases can be considered 🙂

[alexandertrefz]

Now I had the time to test this similarly. I have indeed been using ES2022 as my target. I could have sworn that I tried useDefineForClassFields before I figured out the proposed solution of inlining the Getters & Setters.

But indeed, useDefineForClassFields: false also seems to be working for me.

I am undecided which approach is better, on one hand, just changing the compilation options seems like a cleaner solution, that has less code duplication, etc. On the other hand, it seems slightly more cryptic to depend on the compilation settings to make this work, and should be put into the docs, at the very least.

At the very least it is good to know that it is possible to make it work, with the current version! Thanks @kristilw!

[kristilw]

Thought I had found a nice solution by using the "declare" keyword infront of the property assigment like this:

class MyComponent {
  declare prop: string;
}

but the Angular @Component decorator made typescript ignore the "declare " keyword and the property got included anyways :unamused:

If we arent able to completely remove the properties then using getters and setters seemes like a nice solution :thumbsup: My only suggestion is that (if I understand the code correctly) there isn't any point in writing the full implementation of the getter and setter functions (because they will be overwritten by the @ProxyCmp decorator, and having the implementation written in two different places is confusing). I think the only thing needed to get the typing working is this:

class MyComponent {
  set prop(_: Components.MyComponent['prop']) { }
}

I did a brief test and it seemed to work in my project (just tested a single property by manually editing the proxies.ts file in our build).

What do you think @alexandertrefz ?

[alexandertrefz]

@kristilw You are going through the exact same attempts that i've tried :D

I think leaving it empty is reasonable, as long as the compiler doesn't complain. I'll try this shorter option out, it seems like a decent improvement.

Edit:

Seems to work just fine for me, I would be happy to adjust the PR.