registration[1] is not a function

[liajoy]

Got a problem because of #2144. setTimeout to promise.resolve().then(...) means macro task to micro task, but the micro task will run immediately in script. When trying to get register, the firstNamedDefine can't be got, because it has been destroyed. So this error 'registration[1] is not a function' occurs. The timeline likes:

execute scripnt -> register named module -> clear firstNamedDefine -> script's load event triggered -> get register ->
firstNamedDefine is null

It'll be reproduce by running only running the test suit 'Named System.register'.

I wonder if there's necessary to clear the firstNameDefined by delay? This process seems OK(maybe):

execute script -> register named module -> update firstNamedDefine -> script's load event triggered -> get register -> clear firstNamedDefine -> execute script -> ...

[jolyndenning]

Please provide a demonstration of the issue. You can fork this code sandbox to do so.

The Named System.register tests pass for me locally. Running only the Named Register test suite causes all of them to fail, since the tests rely on order of execution to set up the SystemJS extras.

[liajoy]

Please provide a demonstration of the issue. You can fork this code sandbox to do so.

The Named System.register tests pass for me locally. Running only the Named Register test suite causes all of them to fail, since the tests rely on order of execution to set up the SystemJS extras.

Please see this: https://codesandbox.io/s/nostalgic-microservice-4sp9j?file=/index.html

[jolyndenning]

Please see this: https://codesandbox.io/s/nostalgic-microservice-4sp9j?file=/index.html

Thanks, I'm able to reproduce the error. The reason that we use Promise.resolve() instead of setTimeout() is because we went through a cycle of about 5 bugs with the named register extra where we tried a whole bunch of approaches. See #1984, #2029, #2042, #2074, #2104, #2114, #2121, and #2144.

Because the named register extra has been so problematic, I've stopped using it myself and have encouraged others to stop using it. However, it is still supported and we can look into this bug you've been encountering.

The idea of using Promise.resolve() and setTimeout() was to try to find a reliable way with the event loop to reset firstNamedDefine after the load event fires but before other scripts have a chance to call System.register() with an additional module.

@guybedford can you comment here on this? It seems that neither setTimeout nor Promise.resolve() are the correct approach, and I wonder if there's no solution that relies on the event loop order to try to execute code right after the load event of the script.

[jolyndenning]

Perhaps one solution would be to have script-load.js call a method on the systemjs instance after the load event fires?

systemjs/src/features/script-load.js

Line 71 in 895ebaf

script.addEventListener('load', function () {

Perhaps something like this?

// script-load.js
script.addEventListener('load', () => {
  const register = System.getRegister();

  if (System.scriptLoaded) System.scriptLoaded()
})

// named-register.js
System.prototype.scriptLoaded = () => {
  firstNamedDefine = null;
}

Perhaps that change, combined with switching back to setTimeout instead of Promise.resolve() would fix things?

[guybedford]

@joeldenning thanks for following up on this. What you're suggesting actually does sound like exactly the right approach actually. Perhaps we can turn this into an execComplete hook where the default implementation calls getRegister, something like:

// script-load.js
script.addEventListener('load', function (s) {
  System.execComplete(s.url);
})

It does seem like this gets around the original problem we wanted the timeout for entirely since we can clear everything on completion of all the defines.

Note that we would need to ensure the hook also is called for the eval extension and worker cases as well.

Let me know if that sounds like an approach further, and happy to jump in too as necessary. The replication makes all the difference here so thanks @liajoy for the details.

[guybedford]

finishExec might be more descriptive and slightly shorter.

[jolyndenning]

Oh so we can perhaps remove the Promise.resolve and setTimeout entirely, since the load will cover all timings? That's nice. And good point about the eval and worker cases needing to be updated.

I probably won't work on this today, but might be able to work on it later this week. @guybedford if you have time and interest to work on it before me, definitely do so - I'm not attached to being the one who implements it.

[jolyndenning]

We released a fix for this in https://github.com/systemjs/systemjs/releases/tag/6.9.0. It's still possible to see this error if you're incorrectly using the named-register extra, but the fix should help things for valid calls to System.register

[viT-1]

@joeldenning, with 6.9.0 version bundle by outFile option in tsconfig.json is not supported still?
It is a pity that due to problems with named modules, there is no way to update my project from 5.0.0 version =(

[jolyndenning]

I don't know whether 6.9.0 fixed typescript bundles using the outFile option.