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

Go To Definition doesn't work for certain module patterns #12278

Closed
klinquist opened this issue Nov 15, 2016 · 11 comments
Closed

Go To Definition doesn't work for certain module patterns #12278

klinquist opened this issue Nov 15, 2016 · 11 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed VS Code Tracked There is a VS Code equivalent to this issue

Comments

@klinquist
Copy link

klinquist commented Nov 15, 2016

@waderyan asked me to open up a bug in the TypeScript repo for my comment on this issue:
microsoft/vscode#15004

TypeScript Version: VSCode 1.7.1

Code

myfile.js:

module.exports = function(baseUrl){
  this.apiCall = function(url, cb) {
     let fullUrl = baseUrl + url;
  };

  return this;
}

Expected behavior:

If I let test = require('myfile')(baseUrl) then test.apiCall(url, cb), I should be able to 'go to definition' on apiCall.

Actual behavior:

Cannot go to definition.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 15, 2016

You have to call new test.

@klinquist
Copy link
Author

When not in strict mode, I don't have to return a new constructor for my script to work..

@aluanhaddad
Copy link
Contributor

Irregardless you need to call the function.

@klinquist
Copy link
Author

Sorry - I do call the function (I just updated my test code). Please try it - you'll see that Go To Definition does not work.

@aluanhaddad
Copy link
Contributor

Indeed it does not work.
The following works as expected

module.exports = function (baseUrl) {
  return {
      apiCall: function (url, cb) {
        let fullUrl = baseUrl + url;
      }
  };
};

The issue is that this is typed as any throughout the original function. The actual value of this will vary at runtime depending on the context in which the code is called.

@klinquist
Copy link
Author

Would a let self = this, then returning self make it work?

@mhegazy
Copy link
Contributor

mhegazy commented Nov 15, 2016

Would a let self = this, then returning self make it work?

no. the system currently does not track assignments to this as property declarations. i.e. this.apiCall is not treated as a declaration for a new property called apiCall on this. this is true if you alias this as well (self = this).

you could however, enhance the experience by using JSDoc comments:

/**
 *  @return {{apiCall: (url, cb)=> string;}}
 */
module.exports = function (baseUrl) {
...
}

@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Nov 15, 2016
@abgivant
Copy link

abgivant commented Nov 16, 2016

I use a similar module pattern in an application I'm working on, and the lack of IntelliSense/definitions for those has been an issue for me as well.

Mine follow this basic pattern:

module.exports = function(x, y, z) {
  var exports = {};
  exports.method = function() {
    // Something that might use x, y, or z
  };
  return exports;
}

Would it be possible to do something like include an option in the jsconfig.json file to specify a property name or list of names to track when determining module exports?

Maybe something like this?

{
  "moduleOptions": {
    "exportProperty": ["exports", "this"]
  }
}

@mhegazy
Copy link
Contributor

mhegazy commented Nov 16, 2016

@abgivant, this pattern is actually an easier one to support, we already have plans to do so on the future. it is a general construction patter that is common in JS.
the this one is a bit more tricky though.

@abgivant
Copy link

@mhegazy Ah cool. Is that part of the "expando" discussion, or is there a more specific feature in the works for things like that?

@waderyan waderyan added the VS Code Tracked There is a VS Code equivalent to this issue label Nov 16, 2016
@waderyan
Copy link

Let's move the latter discussion to the new issue #12416

@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
Design Limitation Constraints of the existing architecture prevent this from being fixed VS Code Tracked There is a VS Code equivalent to this issue
Projects
None yet
Development

No branches or pull requests

6 participants