Fix deleting modules after link failure. Resolves #2286.

[joeldenning]

See #2286. Note that this bug only occurs if the link failure occurs in a dependency, instead of directly on the top level load.

[github-actions]

File size impact

Merging issue-2286 into master will impact 3 files in 2 groups.

browser (2/2)

File	raw	gzip	brotli	Event
dist/s.min.js	+31 (7,495)	-1 (2,946)	+3 (2,670)	modified
dist/system.min.js	-15 (11,638)	-7 (4,458)	-4 (4,018)	modified
Total size impact	+16 (19,133)	-8 (7,404)	-1 (6,688)

node (1/1)

File	raw	gzip	brotli	Event
dist/system-node.cjs	-341 (201,382)	-40 (52,150)	-47 (43,797)	modified
Total size impact	-341 (201,382)	-40 (52,150)	-47 (43,797)

extras (0/8)

No impact on files in extras group.

_{Generated by file size impact during size-impact#424460271}

[guybedford]

Nice to see the fix. I'm still a little uncomfortable merging this without getting to the bottom of the root cause behind it.

Specifically:

1. Just a few lines below we have a special tracing path for exceptions (the env.PRODUCTION promise catch case with the onload call), which is clearly the same issue applied to tracing. What also concerns me seeing that previous PR again is that it isn't throwing the link promise anymore so actually changes the behaviour instead of just being a tracing hook.
2. Just a few more lines down is the linkPromise.catch(function () { load.e = null }) case which I believe is the intended path that was supposed to clear this. Again, I'm unclear on why the dep failure is not causing the link promise to fail.

This makes me think perhaps the root cause is actually the tracing path not rethrowing the promise, in which case that path should become:

  process.env.SYSTEM_PRODUCTION && function (err) {
    triggerOnLoad(...);
    throw err;
  }

Perhaps check if something along those lines might work here?

[joeldenning]

Again, I'm unclear on why the dep failure is not causing the link promise to fail.

The reason is that linkPromise only includes the depLoads' instantiate (I) promises, but not the depLoads' link (L) promises. In this case, the dependency is instantiating fine, but failing to link. In my debugging of this, I first expected to see the linkPromise.catch() to execute, but it didn't since linkPromise is fulfilling when a depLoad fails to link.

Perhaps the best solution would be to wait for both the instantiate and link promises of depLoads, and have linkPromise reject if any of those promises reject?

[guybedford]

Thanks for the info, ok then we're still not quite there on quite understanding the logic so should keep digging a bit I think. We cannot wait on the link promise of children because of cycles though.

In this algorithm, the instantiation promise is the promise for the registration of the module, wile the link promise is the promise for the resolve and instantiate of its dependencies.

Let me schedule some time to properly grok this when I can.

[guybedford]

@joeldenning see #2289 for a fix to the onload handlers. Perhaps try running the test case here against that and see if anything changes?

[joeldenning]

Unfortunately the test case I added here is not fixed by the code in #2289.

[guybedford]

Managed to push up a fix for this - thanks for the clear test case and diagnosis here @joeldenning.

[guybedford]

Actually seems this is still your fix, still digging...

[guybedford]

Ok I think I finally got the logic on this - the path you found was exactly correct down to two issues: (1) I dont think it would handle deep errors since linking promises only reflect the first level, and (2) these code paths should directly align with the error tracing hook code paths. In addition fixed up some bugs on the error tracing too.

Thanks gain for the exact isolation and fix on this.