addLoadToLinkSet dependency adding shouldn't be conditional

[guybedford]

The key issue here is that the dependencies of a load added to a linkset are only added to the linkset itself if that load is loaded.

This seems very strange, as the point of a linkset is to track unloaded dependencies.

The issue is that it is linking linksets, where the linkset has a deep dependency on a load record still loading. The reason for this is that the linkset doesn't seem to track deep unloaded load records necessary to be loaded before the linkset is ready - it only seems to track the first-level loaded dependencies.

The suggested change is 3.c of addLoadToLinkSet, removing the check "if load.[[Status]] is loaded" entirely.

Here is a full run of the algorithm on a sample dependency tree of three modules, with and without the change. The load records and their link sets are visualised at each stage of the algorithm, hopefully in a way that should be fairly obvious.

Any questions just ask.

loads/a depends on loads/b
loads/c depends on loads/a
loads/s depends on loads/c and loads/a

Initial loading state:

loads/a (loading): {loads/a}  
loads/c (loading): {loads/c}  
loads/s (loading): {loads/s}

fetched loads/a, loads/c, loads/s
Load succeeded loads/a:

loads/a (loaded): {loads/a loads/b} {loads/c loads/a} {loads/s loads/c loads/a}  
loads/c (loading): {loads/c loads/a} {loads/s loads/c loads/a}  
loads/s (loading): {loads/s loads/c loads/a}  
loads/b (loading): {loads/a loads/b}  

load succeeeded loads/c:

loads/a (loaded): {loads/a loads/b} {loads/c loads/a} {loads/s loads/c loads/a}  
loads/c (loaded): {loads/c loads/a} {loads/s loads/c loads/a}  
loads/s (loading): {loads/s loads/c loads/a}  
loads/b (loading): {loads/a loads/b}

linking {loads/c loads/a}
ISSUE: we have linked C and A before B is loaded!!!!!!

load succeeeded loads/s:

loads/a (loaded): {loads/a loads/b} {loads/s loads/c loads/a}  
loads/c (linked): {loads/s loads/c loads/a}  
loads/s (loaded): {loads/s loads/c loads/a}  
loads/b (loading): {loads/a loads/b}

linking {loads/s loads/c loads/a}

fetched loads/b
load succeeeded loads/b:

loads/a (loaded): {loads/a loads/b}  
loads/b (loaded): {loads/a loads/b}  

linking {loads/a loads/b}

DONE

The correction is then applied, resulting in the algorithm doing:

loads/a, loads/c, loads/s fetched

loads/a (loading): {loads/a}  
loads/c (loading): {loads/c}  
loads/s (loading): {loads/s}

load succeeeded loads/a

loads/a (loaded): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/c (loading): {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/s (loading): {loads/s loads/c loads/a loads/b}  
loads/b (loading): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  

load succeeeded loads/c

loads/a (loaded): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/c (loaded): {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/s (loading): {loads/s loads/c loads/a loads/b}  
loads/b (loading): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  

load succeeeded loads/s

loads/a (loaded): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/c (loaded): {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/s (loaded): {loads/s loads/c loads/a loads/b}  
loads/b (loading): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  

fetched loads/b
load succeeeded loads/b

loads/a (loaded): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/c (loaded): {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  
loads/s (loaded): {loads/s loads/c loads/a loads/b}  
loads/b (loaded): {loads/a loads/b} {loads/c loads/a loads/b} {loads/s loads/c loads/a loads/b}  

linking {loads/a loads/b}

linking {loads/c loads/a loads/b}

linking {loads/s loads/c loads/a loads/b}

Note that we are now linking in the correct order.

If I've missed something please let me know.

[guybedford]

This may well be related to https://github.com/jorendorff/js-loaders/issues/80

[guybedford]

Note that this change does break the first assertion of updateLinkSetOnLoad because after {loads/a loads/b} is linked, loads/b is removed from the {loads/c loads/a loads/b} linkset, so that when the next updateLinkSetOnLoad is called on that linkset, loads/b isn't in it anymore failing the assertion.

I'm not sure what the intention is here. For now, this is working on my test cases, and is the simplest fix I can see, so I'm silencing the assertion in es6-module-loader. More information would be really appreciated in due course.

[guybedford]

I can confirm this change does resolve #80.

[guybedford]

To confirm, the proposed solution here does work out in complex deep shared dependency scenarios.

[guybedford]

This issue is still incredibly worrying.

About 1 in 100 times (maybe even less) I'll see this same test case never call back for loading C in this example with the fix above, making it the only failing test, and I have no idea why. It's rare enough I can't catch it and debug it.

[guybedford]

I'm pretty much 99% sure that my suggested change is a necessary one for the algorithm here.

[guybedford]

Moved to https://bugs.ecmascript.org/show_bug.cgi?id=2603