single-spa / single-spa-angular

Helpers for building single-spa applications which use Angular
Apache License 2.0
201 stars 78 forks source link

Problems with mounting the same parcelConfig multiple times #234

Open joeldenning opened 4 years ago

joeldenning commented 4 years ago

Demonstration

const parcelConfig = singleSpaAngular({...})

const parcel1 = mountRootParcel(parcelConfig, {...})
const parcel2 = mountRootParcel(parcelConfig, {...})

Expected Behavior

When you mount the same single-spa-angular parcelConfig multiple times, multiple independent parcels should be created.

Actual Behavior

When you mount the same single-spa-angular parcelConfig multiple times, only one of them really works. See more at https://single-spa.slack.com/archives/CGETM8T5X/p1593605815279300

This is caused by the opts object being shared between the parcels, but it containing singleton properties. The properties should be changed to be objects/arrays that allow for multiple values (one for each parcel). Specifically opts.bootstrappedModule should not be a single value, but an array/object with multiple values.

This bug also existed in single-spa-react and was fixed in https://github.com/single-spa/single-spa-react/pull/68. The history of it is that many of the single-spa helper libraries were authored at a time when only single-spa applications existed, instead of parcels. Applications are indeed singletons, which is why there is no issue for them.

joeldenning commented 3 years ago

The issue here is that the options object inside of this file is reused when you mount multiple instances of a parcel.

Whenever we modify the options object inside of bootstrap, mount, or unmount, we need to do so in a way that accounts for the possibility of there being multiple parcels sharing the same options object.

Here are the things that need to be figured out:

  1. Should each mounted instance of the parcel have a separate zone identifier?

https://github.com/single-spa/single-spa-angular/blob/a446de57d54ac7a30747284fc7e8c6fd9b35e149/libs/single-spa-angular/src/single-spa-angular.ts#L69 https://github.com/single-spa/single-spa-angular/blob/a446de57d54ac7a30747284fc7e8c6fd9b35e149/libs/single-spa-angular/src/single-spa-angular.ts#L78

  1. Each mounted instance of the parcel should have its own routingEventListener and bootstrappedNgZone:

https://github.com/single-spa/single-spa-angular/blob/a446de57d54ac7a30747284fc7e8c6fd9b35e149/libs/single-spa-angular/src/single-spa-angular.ts#L81 https://github.com/single-spa/single-spa-angular/blob/a446de57d54ac7a30747284fc7e8c6fd9b35e149/libs/single-spa-angular/src/single-spa-angular.ts#L82 https://github.com/single-spa/single-spa-angular/blob/a446de57d54ac7a30747284fc7e8c6fd9b35e149/libs/single-spa-angular/src/single-spa-angular.ts#L144-L145

  1. Each mounted instance of the parcel should have its own bootstrappedModule:

https://github.com/single-spa/single-spa-angular/blob/a446de57d54ac7a30747284fc7e8c6fd9b35e149/libs/single-spa-angular/src/single-spa-angular.ts#L149

One way that we can have separate variables for each mounted instance is to do something like this:

export function bootstrap(options, props) {
  options.instances[props.name || props.appName] = {};
}

export function mount(options, props) {
  const instance = options.instances[props.name || props.appName]

  instance.bootstrappedModule = ...
}

export function unmount(options, props) {
  const instance = options.instances[props.name || props.appName]
  delete instance.bootstrappedModule
}
horlabyc commented 3 years ago

Hi, is there a way out for this already? I am currently working on a project where I need to mount multiple instances of the same parcel on the view at the same time. I see that angular does not allow this. How do I proceed?

joeldenning commented 3 years ago

It's an outstanding bug / limitation. I'd be happy to review a pull request fixing it - my comment above provides some guidance about it

Xsaza commented 3 years ago

Hi, faced same problem, as I can understand this is still issue and it's impossible to use same several parcels in one app on same page?

joeldenning commented 3 years ago

With the current limitation, you can't mount the same parcel multiple times on the page, but you can mount multiple different parcels at the same time on the page.

MRJCrunch commented 3 years ago

Do you have any news? I also have another issue with angular parcels: If I have, for example, the following structure

<div>
  <h1></h1>
  <parcel></parcel>
</div>

as a result the parcel output overwrites the whole content and I get parcel output only without <h1> or other content

<div>
  <content of the parcel>
</div>

It works well with react parcels.

joeldenning commented 3 years ago

@MRJCrunch the problem you described seems unrelated to this github issue. Please create a new issue, with a demonstration of your problem.

VovkGeorgy commented 3 years ago

@joeldenning Hello! I'm invesigating this problem for some time, and noticed that most of problems can be solved outside single-spa-angular, but one change in single-spa-angular is necessary, is to pass a function to the "template" field in singleSpaAngular function ontions, to set unique templates for each parcel from parcel config. Let me explain.

We pass parcelId through the parcel props in the root config:

const elementToMount1 = document.getElementById('element-to-mount-1') as HTMLElement;
const elementToMount2 = document.getElementById('element-to-mount-2') as HTMLElement;

System.import<ParcelConfig>('sec-app').then((app: ParcelConfig) => {
    mountRootParcel(app, { domElement: elementToMount1, parcelId: 'one' });
    mountRootParcel(app, { domElement: elementToMount2, parcelId: 'two' });
});

And then get it in the main.single-spa.ts singleSpaAngular function:

const lifecycles = singleSpaAngular({
  bootstrapFunction: (singleSpaProps: any) => {
    getSingleSpaPropsById(singleSpaProps.parcelId).next(singleSpaProps);
    return platformBrowserDynamic(getSingleSpaExtraProviders()).bootstrapModule(AppModule(singleSpaProps));
  },
  template: (props: any) => {
    return `<app-root-${props.parcelId} />`;
  },
  Router,
  NavigationStart,
  NgZone,
});

And send parcel id to the AppModule wrapper function, where we return a configuren module instance with custom AppComponent selector:

export const AppModule = (props: any) => {
  @NgModule({
    declarations: [
      AppComponent
    ],
    imports: [
      BrowserModule,
      AppRoutingModule
    ],
    providers: [],
  })
  class AppModule implements DoBootstrap {

    constructor() {}

    ngDoBootstrap(appRef: ApplicationRef) {
      appRef.bootstrap(AppComponent, `app-root-${props.parcelId}`);
    }
  }

  return AppModule;
}

We need to change template field type because, when you bootstrap your AppModule and root AppComponent, angular search dom element by AppComponent selector, and it it the same, than it takes only first one. So we need to set different selectors for AppComponent on module boostraping, and they should be same with teplates in singleSpaAngular() config and different from another app parcel.

  class AppModule implements DoBootstrap {

    constructor() {}

    ngDoBootstrap(appRef: ApplicationRef) {
      appRef.bootstrap(AppComponent, `app-root-${props.parcelId}`);
    }
  }

So seems that don't need alot of changes, just update field type in BaseSingleSpaAngularOptions types.ts

export interface BaseSingleSpaAngularOptions {
  template: string | Function;
  domElementGetter?(): HTMLElement;
  bootstrapFunction(props: AppProps): Promise<NgModuleRef<any>>;
}

Update error checking in singleSpaAngular() single-spa-angular.ts

  if (typeof options.template !== 'string' && typeof options.template !== 'function') {
    throw Error('single-spa-angular must be passed options.template string or function');
  }

And call the template function in getContainerElementAndSetTemplate() - dom.ts

 const containerElement = getContainerElement(domElementGetter);
  containerElement.innerHTML = typeof options.template === 'function' ? options.template(props) : options.template;
  return containerElement;

What do you think?

joeldenning commented 3 years ago

single-spa passes a unique name prop to every parcel, so I think you could use that rather than passing an id prop yourself.

There shouldn't be a need for a workaround, the fix for this issue is not hard to implement. We just need to modify single-spa-angular like I've described above. The opts object needs to be reusable across multiple instances, but currently it is not because single-spa-angular modifies it in such a way that each instance of a parcel would overwrite the same properties on the opts object. We need it to instead use objects where the property keys are the parcels' props.name, so that each parcel does not overwrite properties from the others.

If you have interest in submitting a pull request for that, I would be happy to review it. But I don't have plans to work on it myself anytime soon.

TarasovMV commented 3 years ago

I was able to solve the given problem with no changes to the library.

  1. Use ngDoBootstrap in app.module.ts
    
    import {ApplicationRef, DoBootstrap, NgModule} from '@angular/core';
    import {BrowserModule} from '@angular/platform-browser';
    import {AppComponent} from './app.component';

export const AppModule = (id: string) => { @NgModule({ declarations: [AppComponent], imports: [BrowserModule], providers: [], }) class AppModule implements DoBootstrap { constructor() {} ngDoBootstrap(appRef: ApplicationRef) { appRef.bootstrap(AppComponent, example-root-${id}); } } return AppModule; }

2.  You should upgrade file main.single-spa.ts 

import { enableProdMode, NgZone } from '@angular/core'; import { platformBrowserDynamic } from '@angular/platform-browser-dynamic'; import { AppModule } from './app/app.module'; import { environment } from './environments/environment'; import { singleSpaPropsSubject } from './single-spa/single-spa-props';

if (environment.production) { enableProdMode(); }

export const configOptions = (id: string) => ({ bootstrapFunction: (singleSpaProps: any) => { singleSpaPropsSubject.next(singleSpaProps); return platformBrowserDynamic().bootstrapModule(AppModule(id)); }, template: <example-root-${id} />, NgZone, });

3.  And load parcel with System.import

public mount(appName: string, domElement: HTMLElement, id: string = '1'): Observable { return from(System.import(appName)).pipe( tap((app: { configOptions: (id: string) => SingleSpaAngularOptions }) => { const lifecycles = singleSpaAngular(app.configOptions(id)); mountRootParcel(lifecycles, { domElement }); }) ); }

VovkGeorgy commented 3 years ago

@TarasovMV Hello, good suggestion thanks, but can you please provide a repo with this example? Because i got an error when i tried to use singleSpaAngular outside of micro app: Uncaught Error: single-spa-angular: could not retrieve extra providers from the platform injector. Did you call platformBrowserDynamic(getSingleSpaExtraProviders()).bootstrapModule()?

yajneshrai commented 3 years ago

@TarasovMV: Hello! Thanks for your above solution, I was able to have multiple instances of sample app on my UI. However, I've been facing a minor issue - After the apps are mounted using above code, the changes in the angular component inside any apps are not being detected/rendered. I'm not sure if it's something with Zone, if you have faced the same issue or have any insights I'd really appreciate it. Thanks!

gergan commented 2 years ago

Hi, we have some similiar problems, but we use angular elements - @joeldenning should I create a different bug/bugs about them?
I have created a provisional pull-request for the changes, although it is not ready but probably you could comment on the direction and probably on the way to handle it - https://github.com/single-spa/single-spa-angular/pull/397

What's still missing, is the handling of the multiple elements in the options, so unmount does not work for now, but I'll fix this in the next commit. Tests and documentation should be updated also, but I wanted to see what's your reaction on the changes before I update them.

joeldenning commented 2 years ago

You can create a separate github issue if it's for a different problem, but if it's the same problem then we can keep using this issue.

mikecardonar commented 1 year ago

I solved this problem changing the name of the selector main of my apps, for example I used

I had this in main.single-spa.ts

const lifecycles = singleSpaAngular({
  bootstrapFunction: singleSpaProps => {
    singleSpaPropsSubject.next(singleSpaProps);
    return platformBrowserDynamic(getSingleSpaExtraProviders()).bootstrapModule(AppModule);
  },
  template: '<app-root />',
  Router,
  NavigationStart,
  NgZone,
});

I changed this for

const lifecycles = singleSpaAngular({
  bootstrapFunction: singleSpaProps => {
    singleSpaPropsSubject.next(singleSpaProps);
    return platformBrowserDynamic(getSingleSpaExtraProviders()).bootstrapModule(AppModule);
  },
  template: '<app-rep-admin-example />',
  Router,
  NavigationStart,
  NgZone,
});

and I changed the selecctor name too (app.component.ts)

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.scss']
})

now

@Component({
  selector: 'app-rep-admin-example',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.scss']
})

and for last I changed the index.html

<body>
  <app-root></app-root>
</body>

now

<body>
  <app-rep-admin-example></app-rep-admin-example>
</body>

This error happend 'cause single-spa can't use the same parcelId with multiple parcelConfigs, you need to have a unique parceId per application

jamesalmeida-hyper commented 1 year ago

Working with single-spa-react, my team and I encountered an issue where we couldn't instantiate multiple parcels on the same page using another parcel as a wrapper. After conducting extensive research, we discovered that using the wrapWith prop of the Parcel component, as described in the single-spa-react documentation, resolved the problem. By wrapping our parcels with a simple div, we were able to successfully instantiate multiple identical parcels inside a parcel wrapper without any issues.

I believe that our solution using wrapWith in single-spa-react could contribute some ideas to address the problem, or at least prompt an exploration of the wrapWith prop.

kielvi commented 1 year ago

Hey guys, what was the solution for that?

tsmithKBX commented 1 year ago

We are currently trying to do this as well. I found this thread and saw that it was not possible back in 2021 when using single-spa-angular. Has this been fixed? Using a solution mentioned above by using NgDoBootstrap, I am able to get close, but updates in one parcel overwrite another instance of the same parcel config.