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.

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

Salsa - Type discovering for angularjs 1 js controllers #9600

Closed
miguelchico opened this issue Jul 10, 2016 · 12 comments
Closed

Salsa - Type discovering for angularjs 1 js controllers #9600

miguelchico opened this issue Jul 10, 2016 · 12 comments
Labels
Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript

Comments

@miguelchico
Copy link

miguelchico commented Jul 10, 2016

TypeScript Version: 1.8.10

Using the controller above (written in javascript) the type of injected parameters can't be infered.

Code

(function () {
    'use strict';

    angular
        .module('app')
        .controller('TestController', TestController);

    TestController.$inject = ['$q', '$http', '$timeout'];
    /* @ngInject */
    function TestController($q, $http, $timeout) {
        var vm = this;

        activate();

        function activate() {

        }
    }
})();

Expected behavior:
parameters $q, $http and $timeout well typed

Actual behavior:
parameters $q, $http and $timeout of type any

Check: microsoft/vscode#8163

@bmontgom
Copy link

This is a feature I strongly desire. Please remedy this soon!

@githop
Copy link

githop commented Jul 10, 2016

@miguelchico
Copy link
Author

@githop, have you checked the related issue in vscode project?

@bmontgom
Copy link

@githop unfortunately the problem here seems to be that VSCode itself no longer supports Typings as fully as it did a few weeks ago, after a recent update. Check the referenced issue @miguelchico made, he explains in there.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 11, 2016

@miguelchico This has absolutely nothing to do with typings, and everthing to do with how you have structured you code.

  • Angular controllers have no specific interface they are required to implement, they must simply be instantiable view new so there is nothing regarding the parameter types that can be inferred because there are no limitations on them. Even if you wrote you controller inline, a bad practice but useful to illustrate my point, you would see the following
    image
    There is no specific type the function type the framework requires hence the : Function annotation.
  • When you write this code in TypeScript you would do the following (note I am not trying to be idiomatic).
function TestController(
    $q: ng.IQService, 
    $http: ng.IHttpService, 
    $timeout: ng.ITimeoutService) { ... }
  • Of course this is JavaScript so you cannot write type annotations, but the new TypeScript Salsa service understands type annotations in JSDoc comments.
    consider
/**
 * @param {ng.IQService} $q
 * @param {ng.IHttpService} $http
 * @param {ng.ITimeoutService} $timeout
 */
function TestController($q, $http, $timeout) {

which reads the types right out of the .d.ts file and gives you intellisense as shown
image

  • You probably know, but ngInject is not relevant here and is made completely redundant but the explicit $inject property you are specifying. What you have done is good practice, just remove the superfluous @ngInject
/* @ngInject */
function TestController($q, $http, $timeout) {

A couple of notes. I am using [email protected] in VSCode by settings "typescript.tsdk", I have a jsconfig.json file in my project with just an empty object {}.

It is also worth noting that the maintainer of ng-annotate is moving on, see olov/ng-annotate/issues/245.
I think the whole notion of inferring types from parameter names is very unsound and confusing. The fact the you are explicitly setting $inject speaks to this; it is simply unreliable at best. Actually, it is not even inferring types but rather singleton values, which are registered under specific, but arbitrary names.

@bmontgom There are several extensions that support Typings and TSD in VSCode

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 11, 2016

@miguelchico
Copy link
Author

thank for your explanation @aluanhaddad, but my point is still there.

If you check the issue I opened to vscode team, I explained there that for the same code I wrote here (I know @ngInject is not necessary at all for the example) before the TypeScript Salsa service the types of controllers parameter were automatically discovered.

I know I can do the same writing type annotations in JSDoc, but it force me to write a lot of information that was not necessary before.

You can try your example with a previous version of vscode (0.10.9) and you'll see that even without the JSDoc comments, the editor is able to detect the injected parameters in the controller.

This issue is to ask for that behavior again.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 11, 2016

@miguelchico Sorry, I didn't read the VSCode issue. I have since done so and noticed the comment

To be honest, we did something very 'pragmatic'/hackish to enable this in the past.

I imagine they must have had a mapping somewhere that looked like { "$scope", ng.IScopeService, .. }, and while I cannot speak for the TypeScript team in any way, I doubt this kind of thing will be supported. There is no way to even attempt type resolution in these cases without building support for angularjs directly into TypeScript.
Since VSCode is using TypeScript to power it's JavaScript experience, you are going to see less guesswork, but more reliable results in intellisense, this is better in the long run.
You probably want to checkout #6508

@egamma egamma changed the title Type discovering for angularjs 1 js controllers Salsa - Type discovering for angularjs 1 js controllers Jul 13, 2016
@egamma
Copy link
Member

egamma commented Jul 13, 2016

@aluanhaddad Just for some more background. VS Code always used TypeScript to power JavaScript. In the past we used some source rewriting that translates JS to TS behind the scenes and then used TS to get Intellisense etc. As part of this translation we did the pragmatic mapping of $scope -> ng.I{Scope}Service in the generated JS code. Now that TS is supporting JS directly we have switched to this support and could remove the source code rewriting.

@angelozerr
Copy link

I think the only solution to fix this issue, is to wait for #6508 and implement an angular1 plugin like I have done with ternjs https://github.com/angelozerr/angularjs-eclipse/wiki/Javascript-Features#completions

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 14, 2016

@egamma thank you for the explanation and context, it is very enlightening.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 24, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2017

Given how injection work in Angular1, I do not think there is a way for the express that to the TypeScript type system other than hard coding the support in. We have avoided hard coding any behaviors for frameworks, and that has served us well in the past.

A possible solution here is to add this support in a language service extension that users opt-into. the extension would do a pass over input files and add zero-length nodes for types (i.e. synthetic nodes that do not exist in the source), then pass that to the compiler. for instance function TestController($q, $http, $timeout) would result in function TestController($q: ng.IQService, , $http: ng.IHTTPService, , $timeout: ng.ITimeoutService), where the type annotations are just synthesized by the extension. We have done something similar to this with the vue extension, see vuejs/vetur#94. the vue extension would rewrite a .vue file to add a call to vue.extend, then pass that to the compiler, and thus get the expected behavior.

More information about language service plugins can be found at https://github.com/Microsoft/TypeScript/wiki/Writing-a-Language-Service-Plugin

@mhegazy mhegazy closed this as completed Jun 5, 2017
@mhegazy mhegazy added Suggestion An idea for TypeScript Out of Scope This idea sits outside of the TypeScript language design constraints and removed Needs Investigation This issue needs a team member to investigate its status. labels Jun 5, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

8 participants