Skip to content

Use a function declaration vs a function expression to help deal with a reported Atom+IO.js issue. #2522

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

Merged
merged 1 commit into from
Apr 2, 2015

Conversation

CyrusNajmabadi
Copy link
Contributor

No description provided.

@CyrusNajmabadi
Copy link
Contributor Author

View this change with ?w=1 to get a better diff.

@basarat
Copy link
Contributor

basarat commented Mar 27, 2015

closes #2503

@mhegazy
Copy link
Contributor

mhegazy commented Mar 27, 2015

The change looks fine. i am not sure i understand the original issue though :). so which visit was undefined? and how is that fixed?

@basarat
Copy link
Contributor

basarat commented Mar 27, 2015

@mhegazy I don't understand the original reason either. But here it is mentioned again (#2505 (comment))

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 :
image

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:

image

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:

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

Note : I don't see why this error happens:

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)

Using var visit along with visit = function (node) ... also fixes it (coinciding with my understanding of ReferenceError). Surprisingly renaming also fixed it. 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.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 27, 2015

Thanks for the explanation. it is just concerning that this is happening, and i want to make sure we do not have other places where similar behavior could occur.

@DanielRosenwasser
Copy link
Member

Yeah, can we actually link to the bug in io.js and/or Atom? Or has that not been filed?

@vladima
Copy link
Contributor

vladima commented Mar 27, 2015

@basarat Can you tell me how to repro this issue locally? Without known root cause it looks like a magic and I don't like magic

@basarat
Copy link
Contributor

basarat commented Mar 28, 2015

@vladima agreed. Open atom-typescript with atom-typescript. Have a typescript file active in the editor. Press "ctrl+shift+r" to trigger project symbols resolution. This error will occur and get logged into the atom console as a rejected promise.

Tips:

  • "Ctrl+alt+I" to open atom console.
  • Just install atom typescript like you normally would. Then using atoms settings pane you can open it up in atom.
  • feel free to modify code in typescript.js and prjectServices.ts

@basarat
Copy link
Contributor

basarat commented Mar 28, 2015

Open typescript (i.e Microsoft/typescript) in atom. Have a TS file active. And press ctrl+shift+r and this error will not happen.

@basarat
Copy link
Contributor

basarat commented Mar 28, 2015

Here is a gif to help along the repro.

bug

@basarat
Copy link
Contributor

basarat commented Mar 28, 2015

A quick google search

Does show previous bugs along similar lines:
nodejs/node-v0.x-archive#6235
node-inspector/node-inspector#461

@vladima
Copy link
Contributor

vladima commented Mar 29, 2015

Interesting, I can repro the problem on my windows box, however everything works like charm on Linux. Let me check if I can fish out anything interesting

CyrusNajmabadi added a commit that referenced this pull request Apr 2, 2015
Use a function declaration vs a function expression to help deal with a reported Atom+IO.js issue.
@CyrusNajmabadi CyrusNajmabadi merged commit feabcd0 into master Apr 2, 2015
@CyrusNajmabadi CyrusNajmabadi deleted the visitWorkaround branch April 2, 2015 20:30
@MicahZoltu
Copy link
Contributor

@basarat If you don't mind me asking, what tool do you use to create those gifs like the one in #2522 (comment)? Sorry for posting here, wasn't sure how else to ask. Feel free to delete this message.

@basarat
Copy link
Contributor

basarat commented Apr 17, 2015

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
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.

7 participants