-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
angular directive changes, still missing #2605 #2607
Conversation
guess this is related to #2618 |
controller: any, | ||
transclude: ITranscludeFunction | ||
) => void; | ||
link?: IDirectivePrePost; |
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 basically makes link
compatible with anything that doesn't have pre
or post
members. E.g. link : 'bad'
will compile. I think
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.
that's the bad side of Javascript, you can have all, and Typescript won't allow you to have all heh. making it (): IDirectivePrePost;
doesnt help either.
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.
Here : #2605 you had :
interface IDirectiveLink {
(): IDirectiveLinkFunction;
pre?: IDirectiveLinkFunction;
post?: IDirectiveLinkFunction;
}
I take it that (): IDirectiveLinkFunction;
didn't test well?
In this PR : Seems we are saying either you can have old safety for link
OR you can have the new safety for pre/post
. If this is the case I prefer the old safety since link
is used much more than pre/post
. Correct?
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.
yup, but angular docs say that the best practice is to always return an object of pre/post... that's the confusion.
https://docs.angularjs.org/api/ng/service/$compile#comprehensive-directive-api
Best Practice: It's recommended to use the "directive definition object" form.
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 recommended to use the "directive definition object" form.
I am happy with the compile
definition you have added 👍 That can follow the definition object form. Just don't like it for the link
function.
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.
makes sense, will change it then
instanceAttributes?: IAttributes, | ||
controller?: any, | ||
transclude?: ITranscludeFunction | ||
): void; |
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.
Awesome. Thanks. One more fix. Don't mark these arguments as optional (http://definitelytyped.org/guides/best-practices.html callback signatures)
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.
alright
@basarat It seems it's not possible to create an overloaded function declaration, trying to use
interface IDirectiveLinkFn {
(
scope: IScope,
instanceElement: IAugmentedJQuery,
instanceAttributes: IAttributes,
controller: any,
transclude: ITranscludeFunction
): void;
(
scope: IScope,
instanceElement: IAugmentedJQuery,
instanceAttributes: IAttributes,
controller: any
): void;
(
scope: IScope,
instanceElement: IAugmentedJQuery,
instanceAttributes: IAttributes
): void;
(
scope: IScope,
instanceElement: IAugmentedJQuery
): void;
(
scope: IScope
): void;
(): void;
}
but the compiler won't let me use just scope
for example
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 should be just :
interface IDirectiveLinkFn {
(
scope: IScope,
instanceElement: IAugmentedJQuery,
instanceAttributes: IAttributes,
controller: any,
transclude: ITranscludeFunction
): void;
}
Demo that it accommodates all:
interface IDirectiveLinkFn {
(
scope: any,
instanceElement: any,
instanceAttributes: any,
controller: any,
transclude: any
): void;
}
var foo : IDirectiveLinkFn;
foo = ()=>{};
foo = (scope)=>{};
// And so on
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 are right, I'm overthinking this. The next commit should have fixed it.
directive interface definition and tests
@@ -316,6 +316,7 @@ declare module ng { | |||
// see http://docs.angularjs.org/api/ng.$rootScope.Scope | |||
/////////////////////////////////////////////////////////////////////////// | |||
interface IScope { | |||
[index: string]: 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.
@pocesar This doesn't add any value IMHO:
interface IScope1 {
[index: string]: any;
$apply(): any;
}
interface IScope2 {
$apply(): any;
}
var scope1:IScope1;
scope1['foo'] = 123;
var scope2:IScope2;
scope2['foo'] = 123;
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.
on typescript 1.0.1 with --noImplicitAny
, that fails, therefore you should explicitely define that the interface expects anything to be added to it.
test.ts(13,8): error TS7017: Index signature of object type implicitly has an 'any' type.
rootscope <> iscope
angular directive changes, still missing #2605
@pocesar thanks mate! Just wondering about the need for the following: this: IRootScopeService; |
it's part of the rootscope, "this" always point to the scope itself, like $root points to $rootScope, yadda yadda |
Indeed it does. Tested : http://jsfiddle.net/vuy3sv0g/ |
tabs to spaces, fix white space, separate link and compile interfaces to be casteable