Skip to content

Types Map #17468

Merged
RyanCavanaugh merged 5 commits into
microsoft:masterfrom
RyanCavanaugh:newTypesMap
Aug 22, 2017
Merged

Types Map #17468
RyanCavanaugh merged 5 commits into
microsoft:masterfrom
RyanCavanaugh:newTypesMap

Conversation

@RyanCavanaugh
Copy link
Copy Markdown
Member

Fixes #16685

@RyanCavanaugh
Copy link
Copy Markdown
Member Author

RyanCavanaugh commented Jul 28, 2017

/cc @billti @jramsay

Will port this to 2.3 branch for a script-side update to the SDK once this is 👍

Comment thread src/server/editorServices.ts Outdated
}

private loadTypesMap() {
if (!this.host.fileExists(this.typesMapLocation)) {
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.

this.host.readFile method should return undefined if the file doesn't exist, allowing you to avoid an extra file system stat.

Comment thread src/server/editorServices.ts Outdated
return;
}
try {
const raw: TypesMapFile = JSON.parse(this.host.readFile(this.typesMapLocation));
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.

Would it be worth using ts.parseJsonText and ts.convertToObject instead of JSON.parse so that we can get accurate line/column information for invalid JSON in the logs?

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.

Ah, nevermind. I just saw that you want to port this to 2.3, which doesn't have these two methods.

@rbuckton
Copy link
Copy Markdown
Contributor

rbuckton commented Aug 8, 2017

Can you resolve the merge conflict as well?

@RyanCavanaugh
Copy link
Copy Markdown
Member Author

@rbuckton Done + done

@RyanCavanaugh RyanCavanaugh merged commit 15e15ab into microsoft:master Aug 22, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

3 participants