Named register behaviour changes from v6.3.1 to v6.11.0

[eddneal]

Filing an issue here to demonstrate a change in behaviour, not sure if it's an actual issue or just something to do with our implementation, as I can't find any guidance on how named registers should be used (other than that they are discouraged now).

I have inherited a code base that was originally on a very early version of SystemJs and used named registers heavily, I updated this to 6.3.1 a little while back and am now looking at updating to 6.11.0.

It's my understanding that the use case for named registers is to register modules in advance, potentially multiple of them at the same time. I believe performance and reduction of network requests was the intention of the original author of the code I am working on.

Because of this we load scripts with multiple named registers in them which works fine in v6.3.1 but not in 6.11.0, because of the subsequent changes to script-load.js that check for the last script tag and import its src property. Which means that at the time of the named register registration, the path of the bundle of named registers is imported and the dependencies of those named registers are loaded with the parentUrl of the bundle path.

This causes SystemJS Error#3 errors in the console because of incorrect paths, however later usages of the named registers work correctly because the dependencies are resolved with the correct parentUrl that is defined with the named register.

Demonstration

Code Sandbox: https://codesandbox.io/s/interesting-meitner-kp5if

Some debuggers have been added and can see that

• named-register.js - register is called as expected with name https://2vg07.csb.app/js/b/b.js and deps ['../a/a.js'].
• then system.js - register where lastScript.src is https://kp5if.csb.app/js/bundle.js which is then imported
• then later the dependencies of the https://2vg07.csb.app/js/b/b.js named register ( ['../a/a.js']) are resolved with a parentUrl of https://kp5if.csb.app/js/bundle.js

Expected Behavior

When registering a named register there is no attempt to import the path of the file that contains the named register.

Actual Behavior

When registering a named register the path of the file containing the named register is imported with the dependencies of the named register resulting in malformed paths which cause errors.

I think it is possible that our usage of named registers is not aligned with latest version of SystemJs and would be happy to update or remove this, but I figured I would report the change anyway as likely affects others using the extra. From my limited digging it looked like changes could be made to the script tag parsing to only return those that are added by SystemJs? (which I think is the intention).

[joeldenning]

I'm going to spend some time reading through this and diving into the code sandbox (thanks for providing it), but wanted to post links to the relevant PRs related to the named-register extra. There were some buggy behaviors that we fixed that some organizations were accidentally relying upon, so to those companies the feature/patch changes seem like breaking changes even though we deemed them to be bug fixes.

#2329
#2352
#2363

My guess is that your upgrade issue is related to one of these three. Particularly the change from setTimeout to Promise.resolve is the kind of thing that can break things

[joeldenning]

I looked at the codesandbox you provided and the problems are related to whatever is generating the named registers, not with systemjs itself. Typescript, babel, webpack, and other tools all can do this, so I'm not sure what's doing so in the case of bundle.js in your example. It clearly uses webpack for the bundle, but it seems something else probably is generating the System.register("https://2vg07.csb.app/js/b/b.js", ["../a/a.js"] (which is the part that is causing issues).

Since the build process for bundle.js is not shown in the code sandbox, I can't give further insight there. I did fork your code sandbox and get things working by manually fixing up bundle.js to not have the wrong things in it. See https://codesandbox.io/s/inspiring-wescoff-d1myr?file=/js/bundle.js where it has System.register("/js/b/b.js", ["./a/a.js"]

One thing to note is that generally it is not best to bundle systemjs with webpack, since systemjs is the thing that is supposed to load webpack bundles, not the thing that webpack loads. The intentions of many to avoid as many network requests as possible actually often result in worse performance, not better, since SystemJS alone can be cached for a long time but the bundle with systemjs + proprietary code cannot be cached as long since the proprietary code can change frequently. Browsers are very good multiple network requests, even sharing the same TCP/UDP connection over http2/3 (use QUIC for even better perf) for multiplexing. So the concern about 1-2 more network requests is likely based more on developer bias than on measured performance gains (I'd guess a separate script tag is actually faster).

The main thing to clarify is that SystemJS named registers are given arbitrary names that are not treated as urls. In your case, you're using urls as the name, but that is not necessary and the names are not treated as urls. You could just do System.register('a', ...) and System.register('b', ...) rather than making the name appear as a url. It's for this reason that the parentUrl is that of the bundle, not of the name. I do not think that SystemJS ever treated named register names as urls, so I'd be interested to see the code using SystemJS@6.3 that you suggested was doing so.

Perhaps the best next step would be to create a github repo rather than a code sandbox, so that the webpack/typescript/babel/other build can be shown, so that I can diagnose why the named registers are being generated incorrectly.

[eddneal]

Hi Joel, thanks for your time and for the detailed explanation, it is greatly appreciated.

With regards to your comments on the build, we are using the babel plugin to create the named registers which are then concatenated together. The bundle itself also involves simple concatenation, no webpack. The purpose of the named registers and bundle is to support common functionality shared across multiple pages of a PHP app.

With regard to your comment on the code with v.6.3 which uses URLs as named register names with dependencies which resolve in accordance to that URL. This sandbox should demonstrate that and switching to the latest version will introduce the error. As mentioned previously both versions work, its just there is now the error in console with latest version.
https://codesandbox.io/s/sad-silence-r6qlo?file=/index.html

The explanation of named registers names as arbitrary values which aren't treated as URLs was very helpful. This also suggests that it is not possible to use relative imports as the dependencies of named register's, as there is no URL to base this on, correct?

This reminded me of a previous conversation I had with Guy in the SystemJS Gitter (to my shame I had forgotten about this) which explained the same thing.
https://gitter.im/systemjs/systemjs?at=5e5d0c3dec7f8746aaaa58e1

The issue I had at the time was with trying to import named registers via a relative path, which I got around by using the full URL as the named register name. Which is why these named registers are being generated incorrectly as you say.

So it sounds like our implementation which includes this workaround no longer works as this resolution of dependencies using the named register URL names is not by design (apologies, Guy had made that clear previously). Meaning we should completely review our named register usage as this was aligned with the early versions of SystemJs.

[joeldenning]

Thanks for providing the code sandbox demonstrating things being different in systemjs 6.3. I did verify that it's different, but the reason why is actually more complicated than I thought, and unexpected.

The main reason SystemJS 6.12 is behaving differently is due to the auto import feature introduced in systemjs 6.4 behaving differently than a manual import. I won't explain the auto import feature fully here, but basically it attempts to import any modules that were in <script> within the document, without the need of manually calling System.import() afterwards. You can read more about auto import at https://github.com/systemjs/systemjs/releases/tag/6.4.0 #2216 #2210

The other reason is that SystemJS is accidentally using the named register names of modules as the parent url. This is what we were talking about in the last couple comments and I didn't think it was happening, but it is in an accidental way. In the case of a named module, the parentUrl for any relative dependencies should be the URL of the script containing the named module (not the name of the module itself). However, due to the following lines of code, the name of the module is being treated as such.

systemjs/src/system-core.js

Line 145 in 949272a

return Promise.resolve(loader.resolve(dep, id))

I've created a fix for that in the following code sandbox, but unfortunately it cannot work in IE11, since IE11 does not have any way of supporting document.currentScript for async scripts. You can see that fix at https://codesandbox.io/s/red-fire-4wyqs - see line 32 of the custom named-register.js and lines 388-391 of the custom system.js. The solution itself is quite hacky, but does implement the correct behavior. Since I'm unaware of how to do this in IE11, though, I don't have plans on incorporating into SystemJS itself.

Your options

1. Remove named registers. It really is just better not use them
2. Fix your relative URLs in dependencies (../a/a.js) to be correct. The sandboxes you've shared shouldn't ever work even with my fix because the relative urls lop off the /js/ directory when they shouldn't. So it should ./a/a.js rather than ../a/a.js. Then change the names of your modules to not be url-like. For example, System.register('b') rather than `System.register('https://....')
3. Use the custom versions of systemjs and named-register, understanding they won't work in IE11 and are not official or maintained. Then still fix the ../a/a.js to be ./a/a.js

I think any of these options will work, but am least confident in option 2 because i didn't test it fully in my sandbox.

[eddneal]

Once again, thank you so much for your time on this, greatly appreciated.

What you say about the auto import feature makes sense, I ended up digging in those commits while debugging but it was a little over my head :D.

Will take a look at the custom versions you provided (thanks for those), I don't think IE11 is a concern anymore but I would rather look to remove the named registers as have seen you discourage their use a number of times.

Thanks again.