Fix "Module x.js did not instantiate" when name passed to System.register() #1885
Conversation
|
Thanks for the fix here, so the idea is that the named registration should just be an "empty module" and not the "last named registered module" in the bundle. As otherwise it's kind of arbitrary, although we could come up with a consistent convention there. Could you please also add a test for this? |
|
I don't have enough context to understand what you mean by that. Could we chat on gitter? |
|
Sure, I've responded on gitter. |
src/extras/named-register.js
Outdated
| this.registerRegistry[name] = [deps, declare]; | ||
| return register(deps, declare); |
There was a problem hiding this comment.
If you make this return register.call(this, [], function () { return {}; }); that would effectively provide an "empty module" value for the bundle itself.
There was a problem hiding this comment.
Makes sense… more or less, anyway!
There was a problem hiding this comment.
How do I get the actual module to execute?
There was a problem hiding this comment.
As in the test case, it's a layered import process, alternatively the bundle can be loaded with a <script> tag too.
In SystemJS 1.0 we use bundle configuration to create the relation and do this automatically... such an extra could be created for SystemJS 2.0 certainly. See https://github.com/systemjs/systemjs/blob/0.21/docs/config-api.md#bundles.
There was a problem hiding this comment.
Thanks, that's really helpful. I'll try to do that and make sure this edit works and then update the PR.
|
Is there anything else I can do to help get this merged? |
|
Hey @guybedford, could this be merged and released? I've pulling it in via git but am running into some toolchain issues on Zeit Now. |
|
@paulmelnikow thanks for the ping. I'm slowly getting back into things here but will aim to put time aside for a SystemJS release next week. |
|
Thanks again for the PR here, and apologies for the delay :) Will aim to release shortly. |
|
Published in 2.1.2. |
systemjs/systemjs#1885 is merged.
systemjs/systemjs#1885 is merged.
|
Thank you! |
Fix #1884