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

Jump to bottom

Add require.on function to loader. Taken example from Dojo 1.0. Related to Issue #14 #43

Closed
wants to merge 30 commits into from
Closed

Add require.on function to loader. Taken example from Dojo 1.0. Related to Issue #14 #43

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
862be35
added require.on function, utilising example form dojo 1.0 loader
Feb 19, 2016
8b21694
added enum and removed typing- https://github.com/Microsoft/TypeScrip…
Feb 19, 2016
db3e6eb
switched to string literal for type or signal
Feb 19, 2016
3e8ff8c
reorder interface
Feb 19, 2016
977c341
checkpoint with BF
Feb 19, 2016
bd3a739
added node unit tests
Feb 22, 2016
0234801
added functional tests
Feb 22, 2016
12616b9
added functional tests
Feb 22, 2016
4fd26db
fix failed ie tests
Feb 23, 2016
32fd398
update to TS1.8
Feb 24, 2016
6d66b5d
passed noImplicieUseStrict via additionalFlags param in GruntFile
Feb 24, 2016
2b2cea7
tslint update
Feb 25, 2016
1db58ce
updated tslint
Feb 25, 2016
e8bf185
used globalObject work around and move loadNodeModule function
Feb 25, 2016
bfb9ae9
fixed browser tests;
Feb 25, 2016
02c8def
ident to indent, fixed spaces
Feb 25, 2016
ccfe58d
fix tab
Feb 25, 2016
1bf81d6
updated comments from bforbes
Feb 25, 2016
95849c9
moved interfaces to .d.ts file
Feb 25, 2016
773e559
moved interfaces to .d.ts file
Feb 25, 2016
377ba0c
reorder exports
Feb 26, 2016
c600f8c
parent mid in interface
Feb 26, 2016
cdac75a
comment addressing
Feb 26, 2016
640119f
set edit flag to false
Feb 29, 2016
3fe2501
gitignore
Mar 1, 2016
ae80fa2
moved function def
Mar 1, 2016
ce57790
Update .gitignore
Mar 2, 2016
281ffd0
moved loadNodeModule def and added noop
Mar 2, 2016
6c6b04c
Merge branch 'ts1.8' into moduleLoadErrorReporting
Mar 2, 2016
1a77e3b
updated lint error
Mar 2, 2016
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
135 changes: 135 additions & 0 deletions src/interfaces.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
export interface Config {
baseUrl?: string;
map?: ModuleMap;
packages?: Package[];
paths?: { [ path: string ]: string; };
}

export interface Define {
(moduleId: string, dependencies: string[], factory: Factory): void;
(dependencies: string[], factory: Factory): void;
(factory: Factory): void;
(value: any): void;
}

export interface Factory {
(...modules: any[]): any;
}

export interface Has {
(name: string): any;
add(name: string, value: (global: Window, document?: HTMLDocument, element?: HTMLDivElement) => any,
now?: boolean, force?: boolean): void;
add(name: string, value: any, now?: boolean, force?: boolean): void;
}

export interface LoaderPlugin {
load?: (resourceId: string, require: Require, load: (value?: any) => void, config?: Object) => void;
normalize?: (moduleId: string, normalize: (moduleId: string) => string) => string;
}

export interface MapItem extends Array<any> {
/* prefix */ 0: string;
/* replacement */ 1: any;
/* regExp */ 2: RegExp;
/* length */ 3: number;
}

export interface MapReplacement extends MapItem {
/* replacement */ 1: string;
}

export interface MapRoot extends Array<MapSource> {
star?: MapSource;
}

export interface MapSource extends MapItem {
/* replacement */ 1: MapReplacement[];
}

export interface ModuleMap extends ModuleMapItem {
[ sourceMid: string ]: ModuleMapReplacement;
}

export interface ModuleMapItem {
[ mid: string ]: /* ModuleMapReplacement | ModuleMap */ any;
}

export interface ModuleMapReplacement extends ModuleMapItem {
[ findMid: string ]: /* replaceMid */ string;
}

export interface Package {
location?: string;
main?: string;
name?: string;
}

export interface PackageMap {
[ packageId: string ]: Package;
}

export interface PathMap extends MapReplacement {}

export interface Require {
(config: Config, dependencies?: string[], callback?: RequireCallback): void;
(dependencies: string[], callback: RequireCallback): void;
<ModuleType>(moduleId: string): ModuleType;

toAbsMid(moduleId: string): string;
toUrl(path: string): string;
}

export interface RequireCallback {
(...modules: any[]): void;
}

export type SignalType = 'error';

export interface RootRequire extends Require {
has: Has;
on(type: SignalType, listener: any): { remove: () => void };
config(config: Config): void;
inspect?(name: string): any;
nodeRequire?(id: string): any;
undef(moduleId: string): void;
}

// TODO are we still abbreviating these properties?
export interface Module extends LoaderPlugin {
Copy link
Member

Choose a reason for hiding this comment

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

According to our Style Guide, module exports should be ordered alphabetically.

Copy link
Member Author

Choose a reason for hiding this comment

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

meant to do that, will do now

cjs: {
exports: any;
id: string;
setExports: (exports: any) => void;
uri: string;
};
def: Factory;
deps: Module[];
executed: any; // TODO: enum
injected: boolean;
fix?: (module: Module) => void;
gc: boolean;
mid: string;
pack: Package;
req: Require;
require?: Require; // TODO: WTF?
result: any;
url: string;

// plugin interface
loadQ?: Module[];
plugin?: Module;
prid: string;
}

export interface LoaderError extends Error {
src: string;
info: { module: Module, url: string };
Copy link
Member

Choose a reason for hiding this comment

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

Should parentMid be added to the definition of info?

}

export interface ObjectMap { [ key: string ]: any; }

export interface ModuleDefinitionArguments extends Array<any> {
0: string[];
1: Factory;
}
209 changes: 75 additions & 134 deletions src/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,136 +1,31 @@
import {
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to import from a d.ts. It should just be ambient.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow

Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to import anything. If the interface.d.ts is in the scope of the compiler, these types should already be available to you.

Config,
Define,
Factory,
Has,
MapItem,
MapRoot,
MapSource,
ModuleMap,
ModuleMapItem,
Package,
PackageMap,
PathMap,
Require,
RequireCallback,
SignalType,
RootRequire,
Module,
LoaderError,
ObjectMap,
ModuleDefinitionArguments
} from 'interfaces';

// Create ambient declarations for global node.js variables.
declare var process: any;
declare var require: <ModuleType>(moduleId: string) => ModuleType;
declare var module: { exports: any; };

export interface Config {
baseUrl?: string;
map?: ModuleMap;
packages?: Package[];
paths?: { [ path: string ]: string; };
}

export interface Define {
(moduleId: string, dependencies: string[], factory: Factory): void;
(dependencies: string[], factory: Factory): void;
(factory: Factory): void;
(value: any): void;
}

export interface Factory {
(...modules: any[]): any;
}

export interface Has {
(name: string): any;
add(name: string, value: (global: Window, document?: HTMLDocument, element?: HTMLDivElement) => any,
now?: boolean, force?: boolean): void;
add(name: string, value: any, now?: boolean, force?: boolean): void;
}

export interface LoaderPlugin {
load?: (resourceId: string, require: Require, load: (value?: any) => void, config?: Object) => void;
normalize?: (moduleId: string, normalize: (moduleId: string) => string) => string;
}

export interface MapItem extends Array<any> {
/* prefix */ 0: string;
/* replacement */ 1: any;
/* regExp */ 2: RegExp;
/* length */ 3: number;
}

export interface MapReplacement extends MapItem {
/* replacement */ 1: string;
}

export interface MapRoot extends Array<MapSource> {
star?: MapSource;
}

export interface MapSource extends MapItem {
/* replacement */ 1: MapReplacement[];
}

// TODO are we still abbreviating these properties?
export interface Module extends LoaderPlugin {
cjs: {
exports: any;
id: string;
setExports: (exports: any) => void;
uri: string;
};
def: Factory;
deps: Module[];
executed: any; // TODO: enum
injected: boolean;
fix?: (module: Module) => void;
gc: boolean;
mid: string;
pack: Package;
req: Require;
require?: Require; // TODO: WTF?
result: any;
url: string;

// plugin interface
loadQ?: Module[];
plugin?: Module;
prid: string;
}

export interface ModuleMap extends ModuleMapItem {
[ sourceMid: string ]: ModuleMapReplacement;
}

export interface ModuleMapItem {
[ mid: string ]: /* ModuleMapReplacement | ModuleMap */ any;
}

export interface ModuleMapReplacement extends ModuleMapItem {
[ findMid: string ]: /* replaceMid */ string;
}

export interface Package {
location?: string;
main?: string;
name?: string;
}

export interface PackageMap {
[ packageId: string ]: Package;
}

export interface PathMap extends MapReplacement {}

export interface Require {
(config: Config, dependencies?: string[], callback?: RequireCallback): void;
(dependencies: string[], callback: RequireCallback): void;
<ModuleType>(moduleId: string): ModuleType;

toAbsMid(moduleId: string): string;
toUrl(path: string): string;
}

export interface RequireCallback {
(...modules: any[]): void;
}

export interface RootRequire extends Require {
has: Has;
config(config: Config): void;
inspect?(name: string): any;
nodeRequire?(id: string): any;
undef(moduleId: string): void;
}

interface ObjectMap { [ key: string ]: any; }

interface ModuleDefinitionArguments extends Array<any> {
0: string[];
1: Factory;
}

(function (): void {
const EXECUTING: string = 'executing';
const ABORT_EXECUTION: Object = {};
Expand Down Expand Up @@ -241,6 +136,53 @@ interface ModuleDefinitionArguments extends Array<any> {
return has;
})();

let listenerQueues = {};
Copy link
Member

Choose a reason for hiding this comment

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

Immutable, so should be const?

Should be typed as something like this:

const listenerQueues: { [queue: string]: ((error: LoaderError) => void[] } = {};

Also, you use that typing below, so maybe create an interface or type alias for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

forgot that you can add keys to a const object. Thanks


const reportModuleLoadError = function (parent: Module, module: Module, url: string): void {
let parentMid = (parent ? ' (parent: ' + parent.mid + ')' : '');
Copy link
Member

Choose a reason for hiding this comment

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

Why string literal on the next line but not here?

Copy link
Member

Choose a reason for hiding this comment

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

You never change the value of parentMid so why not const?

let message = `Failed to load module ${module.mid} from ${url}${parentMid}`;
Copy link
Member

Choose a reason for hiding this comment

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

Again, no re-assignment, should be const

let error = mix<LoaderError>(new Error(message), {
src: 'dojo/loader',
info: {
module,
url,
parentMid
}
});

if (!emit('error', error)) { throw error; };
};

const emit = function(type: SignalType, args: {}): number | boolean {
let queue: any[] = listenerQueues[type];
Copy link
Member

Choose a reason for hiding this comment

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

With the typing of listenerQueues above, there should be no reason to assert a type here.

let hasListeners = queue && queue.length;

if (hasListeners) {
for (let listener of queue.slice(0)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the reason for copying the queue here?

Copy link
Member Author

Choose a reason for hiding this comment

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

there was a comment on the previous implementation

// notice we run a copy of the queue; this allows listeners to add/remove
// other listeners without affecting this particular signal

If not longer vaild i'm happy to change or add this comment in

Copy link
Member

Choose a reason for hiding this comment

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

Now I get it... ;-) I would just add those comments there, so I don't ask silly questions again!

listener.apply(null, Array.isArray(args) ? args : [args]);
}
}

return hasListeners;
};

const on = function(type: string, listener: (error: LoaderError) => any): { remove: () => void } {
Copy link
Member

Choose a reason for hiding this comment

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

Probably should be (error: LoaderError) => void

let queue = listenerQueues[type] || (listenerQueues[type] = []);

queue.push(listener);

return {
remove() : void {
for (let i = 0; i < queue.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't there be a better way of slicing based on indexOf?

Copy link
Member Author

Choose a reason for hiding this comment

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

like queue.splice(queue.indexOf(listener), 1);

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but you would have to guard the indexOf I think. So doing something like:

const idx = queue.indexOf(listener);
if (idx >= 0) {
    queue.splice(idx, 1);
}

I don't know which is faster, maybe a JSPerf is in order? Likely immaterial, but an indexOf just seems cleaner to me. Which do you prefer?

if (queue[i] === listener) {
queue.splice(i, 1);
return;
}
}
}
};
};

const requireModule: RootRequire =
<RootRequire> function (configuration: any, dependencies?: any, callback?: RequireCallback): Module {
if (Array.isArray(configuration) || /* require(mid) */ typeof configuration === 'string') {
Expand All @@ -254,6 +196,7 @@ interface ModuleDefinitionArguments extends Array<any> {
return contextRequire(dependencies, callback);
};
requireModule.has = has;
requireModule.on = on;

has.add('host-browser', typeof document !== 'undefined' && typeof location !== 'undefined');
has.add('host-node', typeof process === 'object' && process.versions && process.versions.node);
Expand Down Expand Up @@ -364,11 +307,11 @@ interface ModuleDefinitionArguments extends Array<any> {
array && array.forEach(callback);
}

function mix(target: {}, source: {}): {} {
function mix<T extends {}>(target: {}, source: {}): T {
Copy link
Member

Choose a reason for hiding this comment

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

Can we call this assign to match ES6 and dojo/core?

Copy link
Member Author

Choose a reason for hiding this comment

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

happy to do in a follow up PR but isn't that outside the scope of this issue?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Sorry, I got into a stream of consciousness mode.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we should assign and offload to native assign actually, depending on how easy it is. We should open an issue.

for (let key in source) {
(<ObjectMap> target)[key] = (<ObjectMap> source)[key];
}
return target;
return <T> target;
}

function consumePendingCacheInsert(referenceModule?: Module): void {
Expand Down Expand Up @@ -905,8 +848,7 @@ interface ModuleDefinitionArguments extends Array<any> {
let result = loadNodeModule(module.mid, parent);

if (!result) {
let parentMid = (parent ? ' (parent: ' + parent.mid + ')' : '');
throw new Error('Failed to load module ' + module.mid + ' from ' + url + parentMid);
reportModuleLoadError(parent, module, url);
}

return result;
Expand Down Expand Up @@ -951,8 +893,7 @@ interface ModuleDefinitionArguments extends Array<any> {
has('loader-ie9-compat') ? callback(node) : callback();
}
else {
let parentMid = (parent ? ' (parent: ' + parent.mid + ')' : '');
throw new Error('Failed to load module ' + module.mid + ' from ' + url + parentMid);
reportModuleLoadError(parent, module, url);
}
};

Expand Down
Loading