-
Notifications
You must be signed in to change notification settings - Fork 28
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.
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
Add require.on
function to loader. Taken example from Dojo 1.0. Related to Issue #14
#43
Conversation
require.on
function to loader. Taken example from Dojo 1.0. Related to Issue #14
@@ -28,6 +28,11 @@ export interface Has { | |||
add(name: string, value: any, now?: boolean, force?: boolean): void; | |||
} | |||
|
|||
export interface LoaderError extends Error { | |||
src: string; | |||
info: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have an interface rather than using any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the interface is simply an object, does that suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an object, yes, but you're passing specific things with that object. From the call later in the file:
makeSignalError(message, {
module,
url,
parentMid
});
Will the info be different for different errors? Will there always be some properties that will be there?
@@ -1,12 +1,18 @@ | |||
import * as assert from 'intern/chai!assert'; | |||
import * as registerSuite from 'intern!object'; | |||
|
|||
interface LoaderError extends Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be imported from src/loader.ts
. Since it's just an interface, the compiler is smart enough to know that the import only needs to be done within the compiler and won't be added to the resulting JS.
}); | ||
}; | ||
|
||
const signal = function(type: SignalType, args: {}): number | boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this emit
? It would match what most JS event libraries are calling this functionality these days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, however, it isn't exposed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. But for readability's sake and understanding for future generations, emit
makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
|
||
// TODO are we still abbreviating these properties? | ||
export interface Module extends LoaderPlugin { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -1,136 +1,31 @@ | |||
import { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow
There was a problem hiding this comment.
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.
Added dependency to #46 by merging in TS1.8 branch |
Current coverage is
|
@kitsonk @bryanforbes updated PR to include changes from TS1.8 branch. |
Fixes: #14 #42
Pre-req: #46
Can subscribe to loader errors using
require.on('error', callback)
. This returns an object with aremove
function which cancels the subscription.When an error occurs loading a module in the loader the
signal
function checks to see if there are any subscribers to loader errors, if there are, it calls their callback functions, otherwise an error is thrown.