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 doesn't show any outline for this source code #6644

Closed
egamma opened this issue Jan 27, 2016 · 11 comments
Closed

Salsa doesn't show any outline for this source code #6644

egamma opened this issue Jan 27, 2016 · 11 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@egamma
Copy link
Member

egamma commented Jan 27, 2016

From @alexandrudima on January 27, 2016 10:39

Testing #2218

No jsconfig.json

exports.something = function() {
    return true;
}

salsa-outline

Copied from original issue: microsoft/vscode#2427

@billti
Copy link
Member

billti commented Jan 27, 2016

something is not available at the global scope. It is a property of an object. The symbol list generally doesn't show these. Why would this be expected here? Should exports get special behavior (even though they are symbols you cannot just directly reference).

@alexdima
Copy link
Member

Well, imagine the following use-case: I have configured the module system to be commonjs. I am in a consumer of the module and do Ctrl-Space on the module. I get to see something plus possibly many other exported symbols. I then navigate to the producer module and want to iterate through its exported APIs. Not critical, but would be nice :)

@billti
Copy link
Member

billti commented Jan 29, 2016

It seems reasonable. Things exported are conceptually 'top level' in how you thing about a module. Ryan, how hard would it be to surface CommonJS style exports as top level items for navigation?

@billti billti added the Salsa label Jan 29, 2016
@billti billti added this to the Salsa 0.9 milestone Jan 29, 2016
@billti billti assigned RyanCavanaugh and unassigned egamma Jan 29, 2016
@RyanCavanaugh
Copy link
Member

@egamma What API does VS Code use to populate the list?

@billti
Copy link
Member

billti commented Jan 29, 2016

I believe it is the 'navbar' command VSCode side and TypeScript side (or perhaps 'navto'?)

@billti
Copy link
Member

billti commented Jan 29, 2016

Note: Just stepped into it in the debugger - it is 'navbar', which ultimately calls into getNavigationBarItems.

@mhegazy mhegazy added the Bug A bug in TypeScript label Feb 2, 2016
@mhegazy mhegazy modified the milestones: Salsa 0.9, TypeScript 1.8.2 Feb 2, 2016
@billti
Copy link
Member

billti commented Feb 24, 2016

Need to come up with a design for this. This is neither a function declaration, nor a global value, so we don't put it in the nav bar today. Arguably we should treat exports specially here.

@billti
Copy link
Member

billti commented Mar 13, 2016

Changing the navbar for TypeScript and JavaScript would be quite invasive at this late point for 1.8, but seeing as the JavaScript support is quite new, we can special case JavaScript behavior to temporarily make this experience better, then work to align the JavaScript and TypeScript (and 'navbar' and 'navto' logic) longer term.

After discussion with @egamma, @jrieken, and @mhegazy , I've come up with something like can be seen below. The logic is contained in this commit (not yet merged anywhere).

Basically any function/method is a container of other items. They are named by either the given name, or the property/variable they are assigned to (e.g. exports.foo), or just <function> if nothing else can be inferred. If a function expression is named <function> and does not contain other items, then this is not added to the navbar (to avoid pollution with lots of little lambda expressions). This also recognizes define calls and adds it as a module, with either the name given in the first arg, or the containing file name.

It needs a little tidy up (e.g. getters/setters aren't done yet) and hardening, but let me know if this looks good for your needs.

jsnavbar

@jrieken
Copy link
Member

jrieken commented Mar 14, 2016

👍

@egamma
Copy link
Member Author

egamma commented Mar 14, 2016

This looks good and it is a huge improvement 👍

@warpdesign
Copy link

@billti nice! Will it solve problems with closures as well ? See #7134

@billti billti added the Fixed A PR has been merged for this issue label Mar 15, 2016
@billti billti closed this as completed Mar 15, 2016
@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
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

7 participants