[WIP] Fix creation of multiple elements

[gergan]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

[joeldenning]

Thanks for your contribution. The single-spa-angular project needs maintainers and appreciates your work here.

Using mutexes is not necessary and worse for performance since it prevents concurrency. To solve the problem of multiple instances, we can store any instance variables in objects on opts, where the key in the object is the parcel name and the value is the instance variable. See https://github.com/single-spa/single-spa-react/blob/4d77396990aa68284d2dc82a23ff1b3d3a6d6ed6/src/single-spa-react.js#L139-L140 to see how this is done in single-spa-react

[joeldenning]

Allowing template to be a function could be a good change, but doesn't seem related to mounting multiple instances of the same parcel config. Could you separate it into a new PR?

[gergan]

We need the reference to the parcel name in order to read the properties specific to the targeted web component instance (we could, of course generate a property containing the parcel name in some way and insert it in the tag). The problem on the angular side, is that we insert the element over innerHtml and we don't have the correspondent properties directly connected to the angular element, as it is with react for example. We need to recreate the connection between the custom properties and the angular element, which is practically a singleton.

[joeldenning]

I don't think I understand - why would the template need to be different for each instance of a parcel? Doesn't each instance of a parcel create a new instance of the angular element, with the single-spa props already passed to the angular element? I didn't implement the angular elements portion of single-spa-angular, so might be missing something about how it works.

[joeldenning]

This types.ts file should only contain types, not implementations, but you've added implementations to it.

[gergan]

Sorry I could move the mutex implementation, if it is really needed, or remove it - if in fact it is not needed.

[joeldenning]

We should not need a mutex for allowing multiple instances of the same parcel config to be created and used. In single-spa-react, single-spa-vue, etc we solve the problem just by ensuring that the bootstrap, mount, update, and unmount functions are given variables specific to the instance so that they don't interfere with each other. See https://github.com/single-spa/single-spa-react/blob/4d77396990aa68284d2dc82a23ff1b3d3a6d6ed6/src/single-spa-react.js#L139-L140 which shows how we track separate instances on the opts object without resorting to using mutexes that prevent concurrency.

[gergan]

In fact we could not create different instances for the angular elements, because the web components are essentially singletons. In fact this use case was checked for in the bootstrap method, but the check didn't work - it was a bug. The process is asynchronous, so we have had a reproducible race condition, in which we had the module creation called multiple times - just adding the same parcel with the angular element multiple times in a component.

[joeldenning]

because the web components are essentially singletons

I don't understand this, could you clarify? Are you saying that Angular's implementation of the custom element does not allow for multiple instances of the custom element? If so, that is a bug/limitation of Angular and seems unrelated to single-spa-angular.

[gergan]

It is not angular's fault, the browser does not allow for multiple instances - angular just defines a custom component through the browser API (customElement.define)[https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define] and this API throws exception if the element is already defined NotSupportedException The way it is done at least in the examples is to call the define-method in the enclosing angular module.

Something along the lines:

ngDoBootstrap(): void {
    console.log("doing bootstrap in module v1")
    if (!customElements.get('mandant-widget-v1')) {
      customElements.define(
        // This tag we've have provided in `options.template` when called `singleSpaAngularElements`.
        'mandant-widget-v1',
        createCustomElement(MandantWidgetSelectionComponentV1, {injector: this.injector})
      );
      console.log('customElement registered');
    } else {
      console.log('customElement alredy called');
    }
  }

So even if we create the module once again in the bootstrap function the component will not and should not be defined again, as the browser will not allow it. The angular part just generates a bunch of javascript, which is mapped to the new tag through the browser API.

The current single-spa-angular elements implementation also checks if the module is set and does not bootstrap it again, or at least that's the intention of the code - alas as I already said, the implementation has a race condition problem, if we try to insert multiple components at once - the module will be overwritten sometimes or the bootstrap function will not be called in the other cases.

[joeldenning]

Calling customElements.define() multiple times for the same custom element is not valid, but having multiple instances of the custom element in the DOM is valid. Your earlier comment had me thinking that Angular Elements allowed neither, but your most recent comment clarifies that that's not the case.

Coming back to the original comment here, I still don't see any reason why a mutex would be needed.

[joeldenning]

I don't see how this is connected to the original issue of supporting multiple instances of a single-spa-angular element or parcel. I'm not against the feature, but don't think it should be part of this PR

[gergan]

It is probably not essential but in some way fits in the whole problem with multiple angular element instances and single-spa.
As I said before, in angular there is a problem, that we have a divide between the properties and the component, that's a big difference between angular and react for example. On the other hand the angular elements are practically singletons - so we should have a Subject for each web component instance and in some way differentiate between the Subjects. So we pass the parcel name as an attribute to the angular component (over the template as a function) and then we read the instance specific properties from the props service....

generally in the component.html we have something like:
<p [attr.parcel]="parcel" (click)="selected()">works with {{item}}!</p>
and in the component.ts something along the lines:

 @Input()
  public item!: string;

  @Input()
  public parcel!: string;

  public selected = () => {
  };
  private subscription?: Subscription;

  constructor(private _cd: ChangeDetectorRef,
              private propsService: SingleSpaElementPropsService<WrapperParcelProps>) {
  }

  ngOnDestroy(): void {
    if (this.subscription) {
      this.subscription.unsubscribe();
    }
  }

  ngOnInit(): void {
    this.subscription = this.propsService.getProps(this.parcel).subscribe(
      (props: WrapperParcelProps) => {

[joeldenning]

If Angular Elements do not support having multiple instances of the same custom element with different properties, then I don't think we should either in single-spa-angular. Doing so seems like it would be a hacky workaround for a limitation within Angular Elements. I don't know much about Angular elements, so if I'm misunderstanding please let me know.

[gergan]

The problem is not angular elements per se, the web components could use properties just fine. The problem in fact is the way single-spa-angular is implemented - generally using global subject for the props and passing them in parallel to the creation of the components. In single-spa-react for example, if I understand the code correctly, getElementToRender the properties are just passed to the createElement function, so there is no need for some parallel mechanism to transfer the properties to the component itself.

In single-spa-angular elements the creation of the component just assigns the bare template to the innerHTML and the properties are moved through a global subject - that is the recommended way in the help for example - but this works only if there is just one instance of the app/element using this subject.

Adding all the props to the template is probably a better way of doing it, but there is a problem with this - it will not work for all types of properties or at least I don't know how to do it. So that's why I have just used the idea from this comment and created a map of subscribers, which is indeed a very hacky solution...

If you have another idea how to move the passing of the properties inside the creation of the element in the DOM, I'll be glad to implement it...

[joeldenning]

The problem in fact is the way single-spa-angular is implemented - generally using global subject for the props and passing them in parallel to the creation of the components.

Thanks for this explanation, it helped me understand the issue. Instead of using a singleton Subject to pass the props, the props should just be passed through as attributes/properties to the custom element. I wasn't involved in the original implementation of single-spa-angular's angular elements support, so i don't know what it currently does and am fine changing it. But creating a whole props service when we could just use normal properties seems overkill. Custom elements support properties of any data type (only attributes must be strings). So my first thought is that the data should just be passed to the Angular element as a javascript property rather than an HTML attribute.

[gergan]

https://github.com/single-spa/single-spa-react/blob/4d77396990aa68284d2dc82a23ff1b3d3a6d6ed6/src/single-spa-react.js#L139-L140

Thank you for the prompt response. I'll respond to all the comments - I think all of the parts of this request are needed in order to instantiate the same angular element multiple times, because of some the limitations inherent to angular, but probably I have overseen some facts.

[gergan]

I hope I have explained all the problems we have had and how they relate to the code in the pull request. If I have some misunderstanding of the problems, I describe, I'll be more than happy to correct the pull request.

[joeldenning]

I replied to the comments - if Angular itself doesn't support multiple instances of custom elements (each with different properties) then I don't think single-spa-angular should either, since doing so would be against what Angular Elements does and would require us to hack/workaround a lot of things.

[gergan]

I replied to some of your comments, the problem is not angular elements, but the way the properties are currently passed to the component - through a global subject. I hope I explained the situation better this time :)

[joeldenning]

I understand the problem better now, but do not think it's a good idea to create hacky workarounds for complex data types when custom elements already support complex data types via javascript properties on the custom elements.

I understand next to nothing about Angular Elements implementation of custom elements, but my thought is that complex data should just be passed as javascript properties and we should remove the Subject that controls the props, instead preferring just to retrieve the properties on the custom element.

[gergan]

I understand the problem better now, but do not think it's a good idea to create hacky workarounds for complex data types when custom elements already support complex data types via javascript properties on the custom elements.

I understand next to nothing about Angular Elements implementation of custom elements, but my thought is that complex data should just be passed as javascript properties and we should remove the Subject that controls the props, instead preferring just to retrieve the properties on the custom element.

I agree with this stance, that's why I'll close this pull request. What led me to this implementation, was the current implementation and the workarounds in the bug, but as you say it - it is just a tacky workaround.