Salsa: JS support for discovering and acquiring d.ts files#7179
Conversation
(Mostly isolating VS host changes from PR#6448)
(Rules appear to be more strict - this was not caught on a local lint run)
| } | ||
| } | ||
| else if (id === "include") { | ||
| options.include = isArray(jsonTypingOptions[id]) ? <string[]>jsonTypingOptions[id] : []; |
There was a problem hiding this comment.
This needs to check that the array is actually composed only of strings (same on line 621 as well)
- Adding check to ensure TypingOptions 'include' and 'exclude' arrays are composed of strings - Allow leading whitespace when removing comments from json
| if (jsonDict.hasOwnProperty("dependencies")) { | ||
| mergeTypings(Object.keys(jsonDict.dependencies)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Per our meeting with ASP.NET (and for the general developer experience), didn't we want to search for devDependencies also?
There was a problem hiding this comment.
yes - thanks for the reminder! Will add those as well.
There was a problem hiding this comment.
I don't know how relevant it is here, but package.json has peerDependencies and optionalDependencies as well.
There was a problem hiding this comment.
Thanks for drawing my attention to these. Doesn't seem too compelling to add them at this point but will consider adding them in future if we find the current dependencies are falling short.
There was a problem hiding this comment.
No problem. Seemed like you'd want to cover those anyway, since there'll never be overlap in any of these fields and they are treated the same as regular dependencies on a local project. The only difference is when they're used as a sub-dependency dependencies.
There was a problem hiding this comment.
thanks- included these dependencies as well
…t removes both single and multiline comments
…with jsTypings - simplifying typingOptions parsing after associated managed host changes
| return { options, errors }; | ||
| } | ||
|
|
||
| function ConvertJsonOptionToStringArray(optionName: string, optionJson: any, errors: Diagnostic[], func?: (element: string) => string): string[] { |
There was a problem hiding this comment.
Function names begin with lower-case letters
|
👍 |
| let safeList: Map<string>; | ||
| const notFoundTypingNames: string[] = []; | ||
|
|
||
| function tryParseJson(jsonPath: string, host: TypingResolutionHost): any { |
There was a problem hiding this comment.
i would use parseConfigFileTextToJson instead.
There was a problem hiding this comment.
actually readConfigFile is more appropriate.
Salsa: JS support for discovering and acquiring d.ts files
(Isolating and continuing VS host changes from PR#6448)