Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing bug - named AMD modules were instantiated twice. Resolves #2103. #2104

Merged
merged 3 commits into from
Jan 17, 2020
Merged

Fixing bug - named AMD modules were instantiated twice. Resolves #2103. #2104

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fbb9c07
Fixing bug - named AMD modules were instantiated twice. Resolves #2103.
joeldenning Jan 16, 2020
4389585
Self review
joeldenning Jan 16, 2020
95427fd
Self review
joeldenning Jan 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/extras/amd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
throw Error('AMD require not supported.');
}

let tmpRegister;
let tmpRegister, firstNamedDefine;

function emptyFn () {}

Expand Down Expand Up @@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// otherwise AMD takes priority
// no registration -> attempt AMD detection
if (!amdDefineDeps)
Expand Down Expand Up @@ -135,6 +139,13 @@
global.define.amd = {};

function addToRegisterRegistry(name, define) {
if (!firstNamedDefine) {
firstNamedDefine = define;
setTimeout(function () {
firstNamedDefine = null;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

// We must call System.getRegister() here to give other extras, such as the named-exports extra,
// a chance to modify the define before it's put into the registerRegistry.
// See https://github.com/systemjs/systemjs/issues/2073
Expand Down
14 changes: 13 additions & 1 deletion test/browser/named-register.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ok to me.

return System.import('c');
})
.then(function (m) {
Expand Down Expand Up @@ -51,4 +51,16 @@ suite('Named System.register', function() {
assert.ok(m.foo);
});
});

test('loading named define module should work and should not instantiate it twice', function () {
return System.import('fixtures/amd-named-single-execute.js').then(function (m) {
assert.equal(m.default, 'The first named AMD module');
assert.equal(numNamedAMDExecutions, 1);
}).then(function () {
return System.import('fixtures/amd-named-single-execute.js').then(function (m) {
assert.equal(m.default, 'The first named AMD module');
assert.equal(numNamedAMDExecutions, 1);
});
});
});
});
5 changes: 5 additions & 0 deletions test/fixtures/browser/amd-named-single-execute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
var numNamedAMDExecutions = 0;
define('named-amd-single-execute', ['exports'], function(exports) {
numNamedAMDExecutions++;
return 'The first named AMD module';
});