Fixing bug - named AMD modules were instantiated twice. Resolves #2103. #2104
Conversation
| @@ -11,7 +11,7 @@ suite('Named System.register', function() { | |||
|
|
|||
| test('Loading a named AMD bundle', function () { | |||
| return System.import('./fixtures/browser/named-amd.js').then(function (m) { | |||
| assert.equal(Object.keys(m).length, 2); | |||
| assert.equal(m.a, 'b'); | |||
There was a problem hiding this comment.
This test was making a weird assertion that I think was wrong. On the master branch, m is {__useDefault: true, default: {}}. Even though named-amd.js registers two modules that have values in them.
With my changes in this PR, m becomes the c module defined here. This is similar behavior to how the named-register extra keeps track of the firstNamedDefine. I updated the assertions to verify that the c module is being returned now.
| @@ -88,6 +88,10 @@ | |||
| if (register && register[1] === lastRegisterDeclare) | |||
| return register; | |||
|
|
|||
| // If the script registered a named module, return that module instead of re-instantiating it. | |||
| if (firstNamedDefine) | |||
| return firstNamedDefine; | |||
There was a problem hiding this comment.
Before this PR, we were calling createAMDRegister() multiple times for the same AMD module. This is problematic because (1) some modules rely on their code only being executed once, and (2) performance of running the execute function multiple times.
| firstNamedDefine = define; | ||
| setTimeout(function () { | ||
| firstNamedDefine = null; | ||
| }); |
There was a problem hiding this comment.
We shouldn't rely on a setTimeout. Rather we should clear this when we know it is safe to clear it. If this code path is the only knowledge that we should clear it then perhaps we should have a shouldClearNamedDefine variable.
There was a problem hiding this comment.
I thought through how to do this without setTimeout earlier - one approach I considered to was to reset firstNamedDefine inside of getRegister(), or perhaps inside of instantiate.
Could you explain how this shouldClearNamedDefine variable would work?
| @@ -11,7 +11,7 @@ suite('Named System.register', function() { | |||
|
|
|||
| test('Loading a named AMD bundle', function () { | |||
| return System.import('./fixtures/browser/named-amd.js').then(function (m) { | |||
| assert.equal(Object.keys(m).length, 2); | |||
| assert.equal(m.a, 'b'); | |||
See #2103.
@guybedford I'd appreciate your thoughts on this.