Hi @basarat, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

I'm a bit confused. Why is 'visit' giving you an error?

Why is 'visit' giving you an error?

Absolutely no rational reason. I am calling getNamedDeclarations in a simple loop : https://github.com/TypeStrong/atom-typescript/blob/e8a908c884186109e52b78ccecb941b91e96420c/lib/main/lang/projectService.ts#L805

It works fine for most projects including TypeScript :

However for atom-typescript itself, and specifically this source file : https://github.com/TypeStrong/atom-typescript/blob/master/lib/typings/mixto/mixto.d.ts

It gives the error as shown:

ReferenceError: visit is not defined
    at visit (C:\Users\basaratsyed\.atom\packages\atom-typescript\node_modules\typescript\bin\typescript.js:28840:51)
    at visitEachNode (C:\Users\basaratsyed\.atom\packages\atom-typescript\node_modules\typescript\bin\typescript.js:4527:30)
    at Object.forEachChild (C:\Users\basaratsyed\.atom\packages\atom-typescript\node_modules\typescript\bin\typescript.js:4659:24)
    at SourceFileObject.getNamedDeclarations (C:\Users\basaratsyed\.atom\packages\atom-typescript\node_modules\typescript\bin\typescript.js:28796:20)
    at Object.getNavigateToItems (C:\Users\basaratsyed\.atom\packages\atom-typescript\dist\main\lang\projectService.js:600:36)
    at Child.RequesterResponder.processRequest (C:\Users\basaratsyed\.atom\packages\atom-typescript\dist\worker\lib\workerLib.js:38:60)
    at process.<anonymous> (C:\Users\basaratsyed\.atom\packages\atom-typescript\dist\worker\lib\workerLib.js:214:23)
    at process.emit (events.js:119:17)
    at handleMessage (child_process.js:326:10)
    at Pipe.channel.onread (child_process.js:354:11)"__proto__: Object

Fixes that work:

• renaming visit (as this PR does)
• storing visit in an explicit variable outside the forEach (I tried that first to see if my observation was correct).

Running the same code on the mixto.d.ts file alone does not cause it:

So I do not have a small repro for this, other than to run ctrl+shift+r on Atom-TypeScript itself.

@CyrusNajmabadi in case it got lost in my noise. Look at the call stack. I see no reason for visit to be possibly undefined.

ReferenceError: visit is not defined
    at visit (node_modules\typescript\bin\typescript.js:28840:51)
    at visitEachNode (node_modules\typescript\bin\typescript.js:4527:30)
    at Object.forEachChild (node_modules\typescript\bin\typescript.js:4659:24)
    at SourceFileObject.getNamedDeclarations (node_modules\typescript\bin\typescript.js:28796:20)

In this typescript.js file

Hrmm. I'd really like to understand why this error is occurring, rather than just patching around it. Is it possible the generated JS has some oddities around it in this area?

generated JS has some oddities

I don't see how:

The error is ReferenceError. This is the error one gets if there is foo = 123 but no var in strict mode as shown:

"use strict"
foo = 123

C:\Users\basaratsyed\test.js:2
foo = 123
    ^
ReferenceError: foo is not defined
    at Object.<anonymous> (C:\Users\basaratsyed\test.js:2:5)

Not presented in this PR: Using var visit along with visit = function (node) ... also fixes it (coinciding with my understanding of ReferenceError). Surprisingly renaming also fixed it (as presented in this PR). I want to chalk this off as an atom(really iojs or v8) bug i.e. some issue with the name visit in this particular context under some wierd circumstances, that will sort itself out. But thought I'd share a fix for current code.

I've created #2522 as the way to (hopefully) work around this issue, while using the same coding style as the rest of the services layer. Thanks!

Cool ❤️

Does this issue repro with the latest bits from master?

@vladima Yes