Load implementation

[bryanforbes]

Fixes #35.

[kfranqueiro]

I have noticed we've been adding these two declarations in various places (actually, usually when I see us declare define, it includes the amd key). Should we centralize them somewhere? (Am I wrong to be concerned about clobbering/conflicts?)

Edit: Actually, I suppose the one place I saw it in core was in request, which this refactors. I saw it in the crypto API PR in that repo.

[kitsonk]

And do we need an interface somewhere, or do we need a typings from the loader that we should depend on?

[bryanforbes]

I'm not sure if we need to provide typings for loader internals. The problem with providing those is that the typings for node's require and AMD's require are a bit different. Since we're compiling to UMD, most of the need for loader typings is reduced and this module's default function reduce the need for those typings even further.

[kitsonk]

Test cases? 😉

[bryanforbes]

I'll write some test cases tonight. It'll be interesting to test the Node path since Intern runs in an AMD loader...

[bryanforbes]

I finished up the test cases. There's not a ton to check. One thing to note: the coverage data is off in Node because we're using RequireJS which adds some extra stuff to the interpreted file which throws off istanbul. This is fixed once we switch to the new loader.

[kitsonk]

import * as assert from 'intern/chai!assert';

[bryanforbes]

The assert module is not an ES6 module, so it (more than likely) shouldn't be imported with ES6 semantics. This may have been fixed in later builds of TS (I've seen instances where import * as ... isn't callable), but it's still confusing since in ES6, assert would be a non-callable object.

[kitsonk]

Ok.

[kitsonk]

Would it be useful for the tests to also load text resources, instead of just JSON JavaScript Objects? (and actually test loading some JSON too)

Also, I get the reason why, but it is a shame the API is load(require, ...mids); Is there no create way to figure out how to automatically shorten it to load(...mids); or make a context require optional in some way?

[bryanforbes]

I just pushed a change (and rebased on master) so the require argument is an optional first argument. I added tests for both contextual and global-only loading for both Node and AMD.

With regards to text/JSON loading, this module is meant to be a bridge to the module loader. In Node, that means you can actually pull in JSON objects with it, but in the browser (AMD) you can't and you'd have to use a plugin.

[kitsonk]

why not?

export interface Load {
    (require: Require, ...moduleIds: string[]): Promise<any[]>;
    (...modulesIds: string[]): Promise<any[]>;
}

[bryanforbes]

Off the top of my head, I don't remember why I did this. Perhaps I was being clever. When I review this over the weekend, I'll see if I can come up with a better defense ;).

[kitsonk]

I think you implementation logic is 100% correct. I just think the exposed API could be a bit cleaner (though it is technically correct). Let's just admit, the availability of union types of blinded us from thinking of overloading things! I have found myself getting wound into knots with union types only to realises that overloading actually makes things cleaner.

[bryanforbes]

Merged in aa982d9.