List modules in a consistent order

[fredden]

Fixes #23

[krzksz]

Hey, @fredden, thanks for working on this. Maybe instead of creating a new context and undefining all of the modules we could use page.evaluateOnNewDocument to inject a script that hooks into onResourceLoad which already gets a list of module dependencies as it loads? I'm not sure if it is a right way but should result in cleaner code when successful.

[fredden]

I spent a long while working with onResourceLoad initially. The best I could get from this was a load order that's similar to what we have currently - ie, an order that works, but isn't in a consistent order between generations. (It was more stable than current, but not consistent.)

When using onResourceLoad, I was able to detect dependencies the first time something loaded, but not when a second script required the same. For example, with a and b both requiring c: when a gets loaded, c is pulled in first, and onResourceLoad shows that "c is a requirement of a", but when b loads, c is already available, so onResourceLoad wasn't saying anything about c being a requirement of b. To work around this, I tried loading each script in isolation (calling require.undef() on all modules, then watching onResourceLoad to get the tree for that script), but this took far too long. (The website I was using to test with took ~1 minute with no changes, ~1.3 minutes with the approach in this pull request, and ~24 minutes with the approach just described.) Even then, I was getting a little drift with the order produced. The approach in this pull request produces a consistent order every time.

Using a new RequireJS context was necessary because some mix-ins were giving me issues, and I was also having trouble with some of the Magento code that wanted to read the DOM to find x-mage-init scripts. Without calling require.undef(), RequireJS pulled the module from its cache of modules, so define() was never called.

We could move some of the looping into the browser context to tidy this up a little. I wasn't able to pass a Map() between Node & puppeteer, but we could move the whole thing into puppeteer. There was a case where I wanted to 'give up' and return the inconsistent (but working) order, which would be trickier with everything in puppeteer.

--

Looking at the code again just now, I think it should be possible to combine the last two loops into one (dependencyMap to moduleLoadOrder, then moduleLoadOrder to orderedModules --> dependencyMap to orderedModules). That should make the code shorter and may be a little cleaner.

[fredden]

@krzksz, is there anything else you'd like to discuss / have changed in this pull request?

[fredden]

@krzksz can we get this merged in, please?

[mcjwsk]

@krzksz ?

[convenient]

Hey @fredden

Are you using this change currently? Would you consider it stable / worth patching in?

Thanks

[fredden]

@convenient we're using magepack less and less these days. We've been waiting for this change so that our code reviews can be more smooth, but haven't applied it / used a fork for magepack yet. As far as I know, this is stable / ready to merge in / works as advertised.

[convenient]

Thanks @fredden that's much appreciated.

Do you have an alternative to magepack you're using or is it all alternative frontends?

[fredden]

Hyvä theme is our current go-to for new Magento builds. This removes RequireJS from the front-end completely, so magepack and similar are no longer required.

[convenient]

I expected that to be the answer just wanted to confirm 🙂 thanks