Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support input(), output() and model() in Angular elements #53981

Open
aihilali opened this issue Jan 18, 2024 · 20 comments · May be fixed by #55067 or #56728
Open

Support input(), output() and model() in Angular elements #53981

aihilali opened this issue Jan 18, 2024 · 20 comments · May be fixed by #55067 or #56728
Assignees
Labels
area: core Issues related to the framework runtime area: elements Issues related to Angular Elements core: inputs / outputs cross-cutting: signals
Milestone

Comments

@aihilali
Copy link

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

Yes

Description

lets suppose we have this component :

@Component({standalone: true, ...])
export class ToBeAWebComponent {
   anInput = input<string>();
}

by using the @angular/elements package I can export that component as a web-component named : to-be-a-web-component.

const wc = document.querySelector("to-be-a-web-component");
wc.anInput = "aValue"; // this will create a bug and change the type of anInput from InputSignal to string.
// and if the input value is referenced in the template, or any other place, an error will be raised telling that anInput is not a function 

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in (run ng version)

Angular CLI 17.1.0
@angular/core 17.1.0

Anything else?

No response

@devversion devversion self-assigned this Jan 19, 2024
@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime area: elements Issues related to Angular Elements core: inputs / outputs cross-cutting: signals labels Jan 19, 2024
@ngbot ngbot bot modified the milestone: needsTriage Jan 19, 2024
@aihilali
Copy link
Author

It could be better if we have set/update methodes exported for input signals; so we can update them programatically.

@devversion
Copy link
Member

devversion commented Jan 24, 2024

@aihilali Small update on this. I did look into this. Angular Elements require more design and consideration to work well with the signals integration of the framework.

Right now it's not working and we aware of the issue. Thanks for reporting this. There seem to be fundamental design questions with regards to how signal inpus are exposed on the native element instance, how they can be updated etc.

Exposing a set method on the input() directly is likely no the answer here. There are other ways, like exposing setInput from the ComponentRef, or handling this via accessors (like it's done right now when you do wc.anInput = bla)

@chintankavathia
Copy link

@devversion Any specific timeline when we get this fixed?

@ManuelRauber
Copy link

ManuelRauber commented Feb 16, 2024

Hi,

I've noticed this behavior as well. Not only when setting it via JavaScript, but also in HTML, like:

<to-be-a-web-component anInput="hello world"></to-be-a-web-component>

This will also change the anInput field to be a string instead of a signal.

Accessing the signal in the component templates then will lead to an error, because it cannot execute a function on a string.

edit: this also happens in Angular 17.2.

@attilacsanyi
Copy link

I tried playing with Input Transformers, but no luck :(

@darinw
Copy link

darinw commented Feb 17, 2024

I created a simple minimal example of this bug, @aihilali could you add this to your main comment?

https://stackblitz.com/edit/aeh4ae?file=src%2Fapp%2Fangular-view-child.ts

@c-harding
Copy link

c-harding commented Mar 27, 2024

@devversion I’ve got this working using the following NgElementStrategy:

/**
 * Factory that creates new SignalComponentNgElementStrategy instance. Gets the component factory with the
 * constructor's injector's factory resolver and passes that factory to each strategy.
 */
export class SignalComponentNgElementStrategyFactory implements NgElementStrategyFactory {
  constructor(readonly component: Type<object>) {}

  create(injector: Injector) {
    return new SignalComponentNgElementStrategy(this.component, injector);
  }
}

/**
 * Creates and destroys a component ref using a component factory and handles change detection
 * in response to input changes.
 */
export class SignalComponentNgElementStrategy implements NgElementStrategy {
  // Subject of `NgElementStrategyEvent` observables corresponding to the component's outputs.
  private eventEmitters = new ReplaySubject<Observable<NgElementStrategyEvent>[]>(1);

  /** Merged stream of the component's output events. */
  readonly events = this.eventEmitters.pipe(switchMap((emitters) => merge(...emitters)));

  /** Reference to the component that was created on connect. */
  private componentRef: ComponentRef<object> | null = null;

  /** Whether a change detection has been scheduled to run on the component. */
  private scheduledChangeDetectionFn: (() => void) | null = null;

  /** Callback function that when called will cancel a scheduled destruction on the component. */
  private scheduledDestroyFn: (() => void) | null = null;

  /** Initial input values that were set before the component was created. */
  private readonly initialInputValues = new Map<string, any>();

  /**
   * Set of component inputs that have not yet changed, i.e. for which `recordInputChange()` has not
   * fired.
   * (This helps detect the first change of an input, even if it is explicitly set to `undefined`.)
   */
  private readonly unchangedInputs: Set<string>;

  /** Service for setting zone context. */
  private readonly ngZone: NgZone;

  /** The zone the element was created in or `null` if Zone.js is not loaded. */
  private readonly elementZone: Zone | null;

  /** A mirror of the component, for accessing the component’s inputs */
  private readonly componentMirror;

  constructor(
    private component: Type<object>,
    private injector: Injector,
  ) {
    this.componentMirror = reflectComponentType(component);
    this.unchangedInputs = new Set<string>(this.componentMirror?.inputs.map(({ propName }) => propName));
    this.ngZone = this.injector.get<NgZone>(NgZone);
    this.elementZone = typeof Zone === 'undefined' ? null : this.ngZone.run(() => Zone.current);
  }

  /**
   * Initializes a new component if one has not yet been created and cancels any scheduled
   * destruction.
   */
  connect(element: HTMLElement) {
    this.runInZone(() => {
      // If the element is marked to be destroyed, cancel the task since the component was
      // reconnected
      if (this.scheduledDestroyFn !== null) {
        this.scheduledDestroyFn();
        this.scheduledDestroyFn = null;
        return;
      }

      if (this.componentRef === null) {
        this.initializeComponent(element);
      }
    });
  }

  /**
   * Schedules the component to be destroyed after some small delay in case the element is just
   * being moved across the DOM.
   */
  disconnect() {
    this.runInZone(() => {
      // Return if there is no componentRef or the component is already scheduled for destruction
      if (this.componentRef === null || this.scheduledDestroyFn !== null) {
        return;
      }

      // Schedule the component to be destroyed after a small timeout in case it is being
      // moved elsewhere in the DOM
      this.scheduledDestroyFn = schedule(() => {
        if (this.componentRef !== null) {
          this.componentRef.destroy();
          this.componentRef = null;
        }
      }, DESTROY_DELAY);
    });
  }

  /**
   * Returns the component property value. If the component has not yet been created, the value is
   * retrieved from the cached initialization values.
   */
  getInputValue(property: string): unknown {
    return this.runInZone(() => {
      if (this.componentRef === null) {
        return this.initialInputValues.get(property);
      }

      const value: unknown = (this.componentRef.instance as Record<string, unknown>)[property];
      if (isSignal(value)) return value();
      else return value;
    });
  }

  /**
   * Sets the input value for the property. If the component has not yet been created, the value is
   * cached and set when the component is created.
   */
  setInputValue(property: string, value: any, transform?: (value: any) => any): void {
    this.runInZone(() => {
      if (transform) {
        value = transform.call(this.componentRef?.instance, value);
      }

      if (this.componentRef === null) {
        this.initialInputValues.set(property, value);
        return;
      }

      // Ignore the value if it is strictly equal to the current value, except if it is `undefined`
      // and this is the first change to the value (because an explicit `undefined` _is_ strictly
      // equal to not having a value set at all, but we still need to record this as a change).
      if (
        strictEquals(value, this.getInputValue(property)) &&
        !(value === undefined && this.unchangedInputs.has(property))
      ) {
        return;
      }

      this.unchangedInputs.delete(property);

      // Update the component instance and schedule change detection.
      this.componentRef.setInput(property, value);
      this.scheduleDetectChanges();
    });
  }

  /**
   * Creates a new component through the component factory with the provided element host and
   * sets up its initial inputs, listens for outputs changes, and runs an initial change detection.
   */
  protected initializeComponent(element: HTMLElement) {
    const environmentInjector = this.injector.get(EnvironmentInjector);
    const elementInjector = Injector.create({ providers: [], parent: this.injector });
    const projectableNodes = this.componentMirror
      ? extractProjectableNodes(element, this.componentMirror.ngContentSelectors)
      : [];

    this.componentRef = createComponent(this.component, {
      environmentInjector,
      elementInjector,
      projectableNodes,
      hostElement: element,
    });

    this.initializeInputs();
    this.initializeOutputs(this.componentRef);

    this.detectChanges();

    const applicationRef = this.injector.get<ApplicationRef>(ApplicationRef);
    applicationRef.attachView(this.componentRef.hostView);
  }

  /** Set any stored initial inputs on the component's properties. */
  protected initializeInputs(): void {
    this.componentMirror?.inputs.forEach(({ propName, transform }) => {
      if (this.initialInputValues.has(propName)) {
        // Call `setInputValue()` now that the component has been instantiated to update its
        // properties and fire `ngOnChanges()`.
        this.setInputValue(propName, this.initialInputValues.get(propName), transform);
      }
    });

    this.initialInputValues.clear();
  }

  /** Sets up listeners for the component's outputs so that the events stream emits the events. */
  protected initializeOutputs(componentRef: ComponentRef<any>): void {
    const eventEmitters: Observable<NgElementStrategyEvent>[] =
      this.componentMirror?.outputs.map(({ propName, templateName }) => {
        const emitter: EventEmitter<unknown> | OutputEmitterRef<unknown> = componentRef.instance[propName];
        const emitterObservable: Observable<unknown> = isObservable(emitter) ? emitter : outputToObservable(emitter);
        return emitterObservable.pipe(map((value) => ({ name: templateName, value })));
      }) ?? [];

    this.eventEmitters.next(eventEmitters);
  }

  /**
   * Schedules change detection to run on the component.
   * Ignores subsequent calls if already scheduled.
   */
  protected scheduleDetectChanges(): void {
    if (this.scheduledChangeDetectionFn) {
      return;
    }

    this.scheduledChangeDetectionFn = scheduleBeforeRender(() => {
      this.scheduledChangeDetectionFn = null;
      this.detectChanges();
    });
  }

  /** Runs change detection on the component. */
  protected detectChanges(): void {
    if (this.componentRef === null) {
      return;
    }

    this.componentRef.changeDetectorRef.detectChanges();
  }

  /** Runs in the angular zone, if present. */
  private runInZone(fn: () => unknown) {
    return this.elementZone && Zone.current !== this.elementZone ? this.ngZone.run(fn) : fn();
  }
}

c-harding added a commit to c-harding/angular that referenced this issue Mar 27, 2024
Support signal-based components in createCustomElement.

Previously, the signal was overwritten by the input value, losing
reactivity. This change calls setInput on the componentRef.

Also remove onChanges logic in ComponentFactoryStrategy,
because setInput handles this.

DEPRECATED: the injector argument of ComponentNgElementStrategyFactory

The injector argument is no longer needed in the constructor.

Fixes angular#53981
@c-harding c-harding linked a pull request Mar 27, 2024 that will close this issue
14 tasks
@tomasdev
Copy link

https://stackblitz.com/edit/stackblitz-starters-jc7rqz?file=src%2Fmain.ts Another example that reproduces the issue of using signals in Angular Elements

@shadow1349
Copy link

shadow1349 commented May 22, 2024

Hey there, I'm also experiencing the same issue as detailed in @tomasdev's stackblitz. Interestingly, one of mine is working, but others aren't.

  inputOne = input.required<string>();

  inputTwo = input.required<string>();

  inputThree = input(mockObject, {
    transform: (value: string | MyObjectInterface) =>
      typeof value === 'string'
        ? (JSON.parse(value) as MyObjectInterface)
        : value,
  });

inputThree seems to work fine when I call inputThree() but inputOne and inputTwo do not. I even tried to change the first two inputs to match inputThree but it still only complains about inputOne and inputTwo but not three.

@shadow1349
Copy link

After playing with this a bit I think I can kind of see what's happening. Check out this stackblitz: https://stackblitz.com/edit/stackblitz-starters-chkc8v

If you open the console, you'll notice that the TEST WC INPUT 3 console logs the plain value. Whereas the TEST WC INPUT 1 console logs an actual function. So, somewhere in the web component, it is turning that signal into a value rather than a signal input function like it should be. Hopefully, this helps somewhat.

@diesieben07
Copy link

The issue seems to be here:

this.componentRef.instance[property] = value;
this.scheduleDetectChanges();

Changing this to use this.componentRef.setInput(property, value) should be sufficient, which should also remove the need for the manual change detection calls, since setInput does this already.

@diesieben07
Copy link

I looked more into the code of the default ComponentNgElementStrategy and it has a similar problem with outputs. It assumes any component output to be an EventEmitter and calls pipe on it. This also fails for signal based outputs. Instead, the common interface OutputRef must be used, which is implemented both by EventEmitter as well as output signals. It only offers a basic subscribe interface though, so some refactoring would be needed.

@molmosdev
Copy link

Using v18 and input signals still not working in custom elements (@angular/elements). Is it going to be fixed? Thanks :)

@diesieben07
Copy link

diesieben07 commented Jun 23, 2024

I tried working on a pull request to fix this. However this problem goes much deeper and potentially requires deep changes to how ComponentNgElementStrategyFactory works:

  • ComponentNgElementStrategyFactory has its own change detection and scheduling logic, which would have to be removed (because setInput already takes care of change detection). It also has logic to manually all ngOnChanges.
  • The above mentioned logic is referenced in the test cases, which for example test that ComponentNgElementStrategyFactory directly calls ngOnChanges (which would be taken care of by setInput, but the way the test is written this doesn't work, because the test just uses mocks for everything instead of a proper Angular app)
  • ComponentNgElementStrategyFactory still uses the deprecated ComponentFactoryResolver and ComponentFactory. This can be migrated to ComponentMirror fairly easily, however this again requires all the tests to be changed, because they use a mocked ComponentFactory. Additionally, creating a ComponentRef using createComponent requires an EnvironmentInjector, which currently ComponentNgElementStrategy does not have. It might be fairly easy to get, but I do not know how to correctly do it.
  • The NgElementStrategy interface exposes some unfortunate internals:
    • setInputValue for some reason exposes transform, which setInput already takes care of. The correct behavior here is unclear.
    • getInputValue is exposed, but cannot be implemented cleanly with regards to signal inputs. ComponentMirror#inputs does not expose whether an input is a signal (at least not in the public API), so getInputValue cannot know whether the value needs to be "unwrapped" from the signal input or not.

@devversion
Copy link
Member

devversion commented Jun 24, 2024

I've added this project to our team's board (https://github.com/orgs/angular/projects/31/views/2?filterQuery=-status%3ADone+elements&pane=issue&itemId=68529454) and will discuss more with the team on what the timeline on this can be, but no promises. Feel free to continue adding likes to this issue for more attention.

Thanks @diesieben07 for capturing these findings. I've came across similar issues when I prototyped some early support, but it ended up being more work than I was hoping for.

@devversion devversion changed the title signal inputs wierd behavior when used with @angular/elements. Support input(), output() and model() in Angular elements Jun 24, 2024
@molmosdev
Copy link

Thanks, @diesieben07 and @devversion 👏🏻

@xsanda
Copy link

xsanda commented Jun 25, 2024

@devversion just so that it doesn’t get lost: I’ve been using a version of the class in PR #55067 in my project for a few months now, and not had any problems with it.

alxhub added a commit to alxhub/angular that referenced this issue Jun 26, 2024
…duler

The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic
had several downsides:

 * Its behavior different from how Angular components behave in a normal
   template.

   For example, inputs setters were invoked in `NgZone.run`, which (when
   called from outside the zone) would trigger synchronous CD in the
   component, _without_ calling `ngOnChanges`. Only when the custom rAF-
   scheduled `detectChanges()` call triggered would `ngOnChanges` be called.

 * CD always ran multiple times, because of the above. `NgZone.run` would
   trigger CD, and then separately the scheduler would trigger CD.

 * Signal inputs were not supported, since inputs were set via direct
   property writes.

This change refactors the custom element implementation with two changes:

1. `ComponentRef.setInput` is used instead of a custom code path for
   writing inputs.

This allows us to drop all the custom logic related to managing
`ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges`
behavior now matches how the component would behave when _not_ rendered
as a custom element.

2. The custom rAF-based CD scheduler is removed.

Running `NgZone.run` is sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is
called even with no ZoneJS enabled.

As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of `NgZone.run` - see logic above) but the test
didn't catch the issue because it was only spying on `detectChanges()` which
isn't called from `ApplicationRef.tick()`. Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not use `fakeAsync`.

A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with `ngOnChanges` behavior,
where the current behavior could be seen as a bug.

Fixes angular#53981
@alxhub
Copy link
Member

alxhub commented Jun 27, 2024

@diesieben07 hahaha! I took a look at elements this afternoon and encountered literally all of the problems you called out. I wish I'd read your comment beforehand, it would've saved me some time :)

@mauriziocescon
Copy link

Thanks a lot for the PR!

Maybe I'm missing something stupid (then my bad), but I wonder: isn't the PR covering only inputs? Meaning: outputs (model) are still out?

alxhub added a commit to alxhub/angular that referenced this issue Jul 16, 2024
…duler

The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic
had several downsides:

 * Its behavior different from how Angular components behave in a normal
   template.

   For example, inputs setters were invoked in `NgZone.run`, which (when
   called from outside the zone) would trigger synchronous CD in the
   component, _without_ calling `ngOnChanges`. Only when the custom rAF-
   scheduled `detectChanges()` call triggered would `ngOnChanges` be called.

 * CD always ran multiple times, because of the above. `NgZone.run` would
   trigger CD, and then separately the scheduler would trigger CD.

 * Signal inputs were not supported, since inputs were set via direct
   property writes.

This change refactors the custom element implementation with two changes:

1. `ComponentRef.setInput` is used instead of a custom code path for
   writing inputs.

This allows us to drop all the custom logic related to managing
`ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges`
behavior now matches how the component would behave when _not_ rendered
as a custom element.

2. The custom rAF-based CD scheduler is only used when the hybrid scheduler
   is disabled.

Running `NgZone.run` is sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is
called even with no ZoneJS enabled. As a result, the dedicated elements
scheduler is now only used when Angular's built-in scheduler is disabled.

As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of `NgZone.run` - see logic above) but the test
didn't catch the issue because it was only spying on `detectChanges()` which
isn't called from `ApplicationRef.tick()`. Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not use `fakeAsync`.

A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with `ngOnChanges` behavior,
where the current behavior could be seen as a bug.

Fixes angular#53981
alxhub added a commit to alxhub/angular that referenced this issue Jul 16, 2024
…uler

The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic
had several downsides:

 * Its behavior different from how Angular components behave in a normal
   template.

   For example, inputs setters were invoked in `NgZone.run`, which (when
   called from outside the zone) would trigger synchronous CD in the
   component, _without_ calling `ngOnChanges`. Only when the custom rAF-
   scheduled `detectChanges()` call triggered would `ngOnChanges` be called.

 * CD always ran multiple times, because of the above. `NgZone.run` would
   trigger CD, and then separately the scheduler would trigger CD.

 * Signal inputs were not supported, since inputs were set via direct
   property writes.

This change refactors the custom element implementation with two changes:

1. `ComponentRef.setInput` is used instead of a custom code path for
   writing inputs.

This allows us to drop all the custom logic related to managing
`ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges`
behavior now matches how the component would behave when _not_ rendered
as a custom element.

2. The custom rAF-based CD scheduler is only used when the hybrid scheduler
   is disabled.

Running `NgZone.run` is sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is
called even with no ZoneJS enabled. As a result, the dedicated elements
scheduler is now only used when Angular's built-in scheduler is disabled.

As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of `NgZone.run` - see logic above) but the test
didn't catch the issue because it was only spying on `detectChanges()` which
isn't called from `ApplicationRef.tick()`. Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not use `fakeAsync`.

A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with `ngOnChanges` behavior,
where the current behavior could be seen as a bug.

Fixes angular#53981
alxhub added a commit to alxhub/angular that referenced this issue Jul 16, 2024
…uler

The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic
had several downsides:

 * Its behavior different from how Angular components behave in a normal
   template.

   For example, inputs setters were invoked in `NgZone.run`, which (when
   called from outside the zone) would trigger synchronous CD in the
   component, _without_ calling `ngOnChanges`. Only when the custom rAF-
   scheduled `detectChanges()` call triggered would `ngOnChanges` be called.

 * CD always ran multiple times, because of the above. `NgZone.run` would
   trigger CD, and then separately the scheduler would trigger CD.

 * Signal inputs were not supported, since inputs were set via direct
   property writes.

This change refactors the custom element implementation with two changes:

1. `ComponentRef.setInput` is used instead of a custom code path for
   writing inputs.

This allows us to drop all the custom logic related to managing
`ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges`
behavior now matches how the component would behave when _not_ rendered
as a custom element.

2. The custom rAF-based CD scheduler is only used when the hybrid scheduler
   is disabled.

Running `NgZone.run` is sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is
called even with no ZoneJS enabled. As a result, the dedicated elements
scheduler is now only used when Angular's built-in scheduler is disabled.

As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of `NgZone.run` - see logic above) but the test
didn't catch the issue because it was only spying on `detectChanges()` which
isn't called from `ApplicationRef.tick()`. Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not use `fakeAsync`.

A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with `ngOnChanges` behavior,
where the current behavior could be seen as a bug.

Fixes angular#53981
alxhub added a commit to alxhub/angular that referenced this issue Jul 16, 2024
…uler

The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic
had several downsides:

 * Its behavior different from how Angular components behave in a normal
   template.

   For example, inputs setters were invoked in `NgZone.run`, which (when
   called from outside the zone) would trigger synchronous CD in the
   component, _without_ calling `ngOnChanges`. Only when the custom rAF-
   scheduled `detectChanges()` call triggered would `ngOnChanges` be called.

 * CD always ran multiple times, because of the above. `NgZone.run` would
   trigger CD, and then separately the scheduler would trigger CD.

 * Signal inputs were not supported, since inputs were set via direct
   property writes.

This change refactors the custom element implementation with two changes:

1. `ComponentRef.setInput` is used instead of a custom code path for
   writing inputs.

This allows us to drop all the custom logic related to managing
`ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges`
behavior now matches how the component would behave when _not_ rendered
as a custom element.

2. The custom rAF-based CD scheduler is only used when the hybrid scheduler
   is disabled.

Running `NgZone.run` is sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is
called even with no ZoneJS enabled. As a result, the dedicated elements
scheduler is now only used when Angular's built-in scheduler is disabled.

As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of `NgZone.run` - see logic above) but the test
didn't catch the issue because it was only spying on `detectChanges()` which
isn't called from `ApplicationRef.tick()`. Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not use `fakeAsync`.

A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with `ngOnChanges` behavior,
where the current behavior could be seen as a bug.

Fixes angular#53981
alxhub added a commit to alxhub/angular that referenced this issue Jul 17, 2024
…duler

The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic
had several downsides:

 * Its behavior different from how Angular components behave in a normal
   template.

   For example, inputs setters were invoked in `NgZone.run`, which (when
   called from outside the zone) would trigger synchronous CD in the
   component, _without_ calling `ngOnChanges`. Only when the custom rAF-
   scheduled `detectChanges()` call triggered would `ngOnChanges` be called.

 * CD always ran multiple times, because of the above. `NgZone.run` would
   trigger CD, and then separately the scheduler would trigger CD.

 * Signal inputs were not supported, since inputs were set via direct
   property writes.

This change refactors the custom element implementation with two changes:

1. `ComponentRef.setInput` is used instead of a custom code path for
   writing inputs.

This allows us to drop all the custom logic related to managing
`ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges`
behavior now matches how the component would behave when _not_ rendered
as a custom element.

2. The custom rAF-based CD scheduler is removed in favor of the main Angular
   scheduler, which now handles custom elements as necessary.

Running `NgZone.run` is sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is
called even with no ZoneJS enabled. As a result, the dedicated elements
scheduler is now only used when Angular's built-in scheduler is disabled.

As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of `NgZone.run` - see logic above) but the test
didn't catch the issue because it was only spying on `detectChanges()` which
isn't called from `ApplicationRef.tick()`. Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not use `fakeAsync`.

A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with `ngOnChanges` behavior,
where the current behavior could be seen as a bug.

Fixes angular#53981
alxhub added a commit to alxhub/angular that referenced this issue Jul 17, 2024
…duler

The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic
had several downsides:

 * Its behavior different from how Angular components behave in a normal
   template.

   For example, inputs setters were invoked in `NgZone.run`, which (when
   called from outside the zone) would trigger synchronous CD in the
   component, _without_ calling `ngOnChanges`. Only when the custom rAF-
   scheduled `detectChanges()` call triggered would `ngOnChanges` be called.

 * CD always ran multiple times, because of the above. `NgZone.run` would
   trigger CD, and then separately the scheduler would trigger CD.

 * Signal inputs were not supported, since inputs were set via direct
   property writes.

This change refactors the custom element implementation with two changes:

1. `ComponentRef.setInput` is used instead of a custom code path for
   writing inputs.

This allows us to drop all the custom logic related to managing
`ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges`
behavior now matches how the component would behave when _not_ rendered
as a custom element.

2. The custom rAF-based CD scheduler is removed in favor of the main Angular
   scheduler, which now handles custom elements as necessary.

Running `NgZone.run` is sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is
called even with no ZoneJS enabled. As a result, the dedicated elements
scheduler is now only used when Angular's built-in scheduler is disabled.

As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of `NgZone.run` - see logic above) but the test
didn't catch the issue because it was only spying on `detectChanges()` which
isn't called from `ApplicationRef.tick()`. Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not use `fakeAsync`.

A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with `ngOnChanges` behavior,
where the current behavior could be seen as a bug.

Fixes angular#53981
@snortblt
Copy link

snortblt commented Jul 19, 2024

This is a pretty significant problem for us. After some significant PoC work to vet custom elements integration, we've implemented several components using signals, assuming that an input is an input. This is a major shortcoming and doesn't seem to be documented anywhere (except here). Can we at least get this issue added to the docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment