[piral-ng] When one Angular module is unloaded, all other modules are incorrectly torn down as well

[Siphalor]

Bug Report

Prerequisites

• Can you reproduce the problem in a MWE?
• Are you running the latest version?
• Did you perform a search in the issues?

Environment Details and Version

Reproducible with:

piral-core and piral-ng: 0.15.5 (any 0.15 version should do).
OS: tested on Win10 and Linux Mint
Browser: tested with latest Edge and Firefox 

Description and steps to reproduce

Scenario:
There is an Angular pilet using piral-ng that provides components from different modules all defined using defineNgModule.
For easy reproducibility load one extension from each module into the shell.

1. Perform an action that unloads one of the extension slots.
2. piral-ng will not only correctly destroy that component and the now unused module, but also all other Angular modules loaded via piral-ng.
3. The other extension slot's DOM will be empty, since the Angular component got wrongly unloaded.
4. Trying to unload that other extension as well, will result in a hard React crash, since React still thinks that the component is loaded.

Expected behavior

When all components of an Angular module go out of scope, only that module should be unloaded.

Actual behavior

All Angular modules and components will be unloaded/unmounted.

Analysis

This is the code called when a piral-ng component is to be unmounted:

piral/src/converters/piral-ng/src/module.ts

Lines 74 to 85 in 143d72e

	for (let i = this.refs.length; i--; ) {
	const [sourceComponent, sourceNode, ref] = this.refs[i];
	if (sourceComponent === component && sourceNode === node) {
	ref.destroy();
	this.refs.splice(i, 1);
	}
	}
	if (this.refs.length === 0) {
	teardown(BootstrapModule);
	}

This method correctly tries to tear down the module if no other component instances of that module are left.

The teardown method looks like this:

piral/src/converters/piral-ng/src/startup.ts

Lines 55 to 66 in 143d72e

	export function teardown(BootstrapModule: any) {
	const runningModuleIndex = runningModules.findIndex(([ref]) => ref === BootstrapModule);
	if (runningModuleIndex !== -1) {
	const [, , platform] = runningModules[runningModuleIndex];
	runningModules.splice(runningModuleIndex, 1);
	if (!platform.destroyed) {
	platform.destroy();
	}
	}
	}

The code tries to unload the platform of the currently running module, which is also technically correct.

But what are the platforms used?

piral/src/converters/piral-ng/src/startup.ts

Lines 16 to 21 in 143d72e

	function startNew(BootstrapModule: any, context: ComponentContext, ngOptions?: NgOptions) {
	const path = context.publicPath || '/';
	const platform = platformBrowserDynamic([
	{ provide: 'Context', useValue: context },
	{ provide: APP_BASE_HREF, useValue: path },
	]);

They are platformBrowserDynamic platforms which looks ok at first too.

However, looking at Angular's implementation reveals the following:

https://github.com/angular/angular/blob/2fc5b70fcedb8ac35b825b245c0ae394dc125244/packages/platform-browser-dynamic/src/platform-browser-dynamic.ts#L33-L34

https://github.com/angular/angular/blob/2fc5b70fcedb8ac35b825b245c0ae394dc125244/packages/core/src/application_ref.ts#L272-L293

Looking closely at this, you'll notice that platformBrowserDynamic by default always returns the same instance!
That means that destroying the platform of one module, you'll destroy the platform of all modules, actually destroying every Angular component and module that's loaded.

Possible Solutions

1. The easy but dumb solution

Let everything just run on a single platform and don't destroy it.
This has a lot of bad implications for performance and sandboxing of the pilets and probably isn't really worth much.

2. The intended way

From looking at the code, I can guess that it was intended for every module to run on its own platform.
With the help of a bunch of Angular internals (namely ALLOW_MULTIPLE_PLATFORMS as seen above) one can enable multi-platform behavior for the platform factory.

Since I've already gotten all the way there, I'm going to submit a pull request for this solution shortly.

[FlorianRappl]

Yeah that's bad - thanks for spotting. Also awesome to provide these solutions - I agree going for (2) would be superb. A PR is much appreciated! 🍻