Skip to content

Refactor navigation bar #8958

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

Closed
wants to merge 12 commits into from
Closed

Refactor navigation bar #8958

wants to merge 12 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 3, 2016

Fixes #8779, #5258, #4191, and #8218.

Gets us closer to #7523 (combines TS and JS code paths for navigation bar).

This was referenced Jun 3, 2016
@ghost ghost force-pushed the navbar_refactor_2.0 branch from 11b7291 to 89a5d6d Compare June 3, 2016 15:46
@ghost ghost force-pushed the navbar_refactor_2.0 branch from 89a5d6d to 0774892 Compare June 3, 2016 16:07
*/
interface NavNode {
node: Node;
additionalNodes: Node[]; // May be missing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the comment is true, I'd remove the comment, and write additionalNodes?: Node[]; instead. (This would likely be caught if we could compile the compiler under strict null checks)

function createNavNode(parent: NavNode, node: Node): NavNode {
const navNode: NavNode = {
node,
additionalNodes: undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this field is optional, you may just omit it from this initializer, unless there's a perf-related reason not to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably will have better perf

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably will have better perf which way?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 10, 2016

@Andy-MS, can we get some perf numbers of how the new code fairs? i would pick a couple of files for ts and js, small (tsc.ts/ some small js file) and large (checker.ts / typescriptServices.js), and get how long it took us to generate nav bar items, possibly through the tsserver request duration, before and after the change. i would also like see the memory consumption comparison.

@ghost ghost mentioned this pull request Jun 13, 2016
@ghost
Copy link
Author

ghost commented Jun 13, 2016

I've managed to get the time taken down, but it is still about 50% higher than master since we now traverse the whole AST looking for declarations. (In comparison, #8648 takes 300% more time than master!) I'll work with @yuit tomorrow to fully investigate.

@ghost ghost mentioned this pull request Jun 16, 2016
@ghost
Copy link
Author

ghost commented Jun 16, 2016

Closed in favor of #9220

@ghost ghost closed this Jun 16, 2016
@mhegazy mhegazy deleted the navbar_refactor_2.0 branch November 2, 2017 21:02
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation bar: functions nested in methods
3 participants