fix: karma.loaded() started too early (#2955)

[jehon]

On module, you need to wait explicitely on modules being loaded before
starting the tests.
This commit:

• wait for modules to be loaded
• add a test for modules

This is the same as #2956, but with Google CLA ok

[jehon]

This is the same merge request as #2956, but where cla/google could pass.
In the previous pull request, some comments where made:

• being compatible with IE7 (I think it is ok)
• is this new loading system ok (I think it is mandatory to allow es6 modules)
• do we wait for "loaded" or "domContentLoaded" (I think domContentLoaded gives the minimum change necessary for this fix)

[jehon]

The third point is ok (see conversation here #2956 (review))

[jehon]

@dignifiedquire
You validated this other PR #2834, which is basically doing the same thing.

The difference is:

• This one use DomLoadedEvent
• The 2834 is using script with "nomodule" and "type=module", but the tests are not passing, so some more work is necessary

From my point of view, the two commits changes a bit the karma behavior...

Which one of the two is better for you?

[lastmjs]

@jehon I think there might be a much simpler fix to this that would be backwards-compatible. I'm not sure if it is in the spec, but if you replace in context.html:

<script type="text/javascript">
    window.__karma__.loaded();
</script>

with

<script type="module">
    window.__karma__.loaded();
</script>

then everything seems to load correctly. The problem without type="module" is that window.__karma__.loaded(); is getting called before whatever modules the user is importing just before that call. Adding type="module" seems to ensure that the scripts execute in order.

[lastmjs]

Actually, looks like my suggestion is nearly exactly what is happening here: https://github.com/karma-runner/karma/pull/2834/files

@jehon, What is the difference between your pull request and #2834 ? Seems like they are accomplishing the same task, though #2834 seems the more standards-compliant way to accomplish it if module loading of script tags changes based on the scripts importing modules or not

[jehon]

When I started this, I didn't knew some works was done there.

PS: And you also need a <script nomodule> for it to works on older browsers too.

At my understanding, I am not sure that the "module" scripts are loading in order. I didn't see any garantee that the "inline" script would run after all the loaded modules. I think that if you have a module loading slowly, it could run after the "__karma__" stuff. But I am not sure about this. But I am sure that the DomContentReady should fire when all modules are loaded, and that this is compatible with all supported browsers.

[lastmjs]

#2834 Has the nomodule script there as well. I'm not sure about the order, but I believe the scripts will run in order if they are both modules

[jehon]

Reading the W3C specs, I find that:

If the async attribute is present, then the script will be executed asynchronously, as soon as it is available.
(from https://www.w3.org/TR/html50/scripting-1.html#attr-script-async)

PS: module scripts are always async

"As soon as it is available" is a bit confusing. But to my point of view, it does not implies that the scripts will run in order.

[lastmjs]

Ah, I see. I'm not looking at the spec, but from reading some documentation I think type module makes the script defer by default, not become async: https://jakearchibald.com/2017/es-modules-in-browsers/

For example, you can still put the async attribute on a type module script

[lastmjs]

Deferred scripts maintain their order with other deferred scripts, and so if you have multiple type module scripts defined, they will all load in the order written in the html

[lastmjs]

If what I've said is true, #2834 should work by maintaining the order of the scripts, since they are of type module meaning they are deffered

[jehon]

Ah, ok. So the other solution seem more compatible...
Ok for me. Let's them go further.

[johnjbarton]

the other solution seem more compatible...