Skip to content

Language Service host API cleanup#1735

Merged
mhegazy merged 31 commits into
masterfrom
LSAPICleanup
Feb 3, 2015
Merged

Language Service host API cleanup#1735
mhegazy merged 31 commits into
masterfrom
LSAPICleanup

Conversation

@mhegazy

@mhegazy mhegazy commented Jan 20, 2015

Copy link
Copy Markdown
Contributor

Few cleanup changes to the host API also adding a helper function to get the default library path for node packages.

… is never honored by the LS.

The LS operate only on a fixed list of files initialized by the host's getScriptFileNames(), if a file is not in this list, it will not be queried from the host, rather considered a missing file. Thus the result of getDefaultLibFilename()is either in the list, or will be ignored.
Conflicts:
	src/services/services.ts
@mhegazy

mhegazy commented Jan 20, 2015

Copy link
Copy Markdown
Contributor Author

@csnover this change simplifies the host API, take a look and let me know if you see other inconsistencies. still to come is documentation for these APIs.

Comment thread src/services/services.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just noticed, put a semicolon.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bumping up the API version will require updating the check on the managed side as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

reverted.

@DanielRosenwasser

Copy link
Copy Markdown
Member

Looks good, once the minor changes I've suggested are addressed, 👍

@csnover

csnover commented Jan 20, 2015

Copy link
Copy Markdown
Contributor

It will be a few days for additional feedback from me. It’s up to you if you want to wait :)

Comment thread src/services/services.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i'm not sure what this means, or why it is correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1. not sure what we are checking here and why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

checking if we are in node. i am open to suggestions.. this is how we do it in sys anyways.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you just want to use __dirname then check typeof __dirname. This current line isn’t a good check for being in a Node.js environment. You can get the same invalid answer with a define(function (require, exports, module) {}) AMD wrapper or if someone just pollutes some globals. Probably the “best” check for knowing if you are in an environment that is intentionally trying to quack like Node.js is typeof process !== 'undefined' && process.versions && process.versions.node but in any case actually trying to detect the API you want to use is best.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed to check for __dirname

@steveluc

Copy link
Copy Markdown
Contributor

Good simplification. Did you check whether WScript will also give you the directory of the executing .js file?

👍

@JsonFreeman

Copy link
Copy Markdown
Contributor

👍

mhegazy added a commit that referenced this pull request Feb 3, 2015
Language Service host API cleanup
@mhegazy
mhegazy merged commit bbbec22 into master Feb 3, 2015
@mhegazy
mhegazy deleted the LSAPICleanup branch February 3, 2015 03:04
@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.