Salsa: JS support for discovering and acquiring d.ts files #7179

jramsay · 2016-02-22T05:39:47Z

Salsa: JS support for discovering and acquiring d.ts files
(Isolating and continuing VS host changes from PR#6448)

(Mostly isolating VS host changes from PR#6448)

(Rules appear to be more strict - this was not caught on a local lint run)

RyanCavanaugh · 2016-02-22T19:48:04Z

src/compiler/commandLineParser.ts

+                        }
+                    }
+                    else if (id === "include") {
+                        options.include = isArray(jsonTypingOptions[id]) ? <string[]>jsonTypingOptions[id] : [];


This needs to check that the array is actually composed only of strings (same on line 621 as well)

good idea - done

- Adding check to ensure TypingOptions 'include' and 'exclude' arrays are composed of strings - Allow leading whitespace when removing comments from json

billti · 2016-02-23T03:27:15Z

src/services/jsTyping.ts

+                if (jsonDict.hasOwnProperty("dependencies")) {
+                    mergeTypings(Object.keys(jsonDict.dependencies));
+                }
+            }


Per our meeting with ASP.NET (and for the general developer experience), didn't we want to search for devDependencies also?

yes - thanks for the reminder! Will add those as well.

I don't know how relevant it is here, but package.json has peerDependencies and optionalDependencies as well.

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.

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.

thanks- included these dependencies as well

…t removes both single and multiline comments

…with jsTypings - simplifying typingOptions parsing after associated managed host changes

RyanCavanaugh · 2016-02-26T18:33:27Z

src/compiler/commandLineParser.ts

@@ -682,4 +688,28 @@ namespace ts {

        return { options, errors };
    }
+
+    function ConvertJsonOptionToStringArray(optionName: string, optionJson: any, errors: Diagnostic[], func?: (element: string) => string): string[] {


Function names begin with lower-case letters

…to merge in if present.

jramsay · 2016-02-26T23:53:51Z

👍

mhegazy · 2016-02-27T00:35:31Z

src/services/jsTyping.ts

+    let safeList: Map<string>;
+    const notFoundTypingNames: string[] = [];
+
+    function tryParseJson(jsonPath: string, host: TypingResolutionHost): any {


i would use parseConfigFileTextToJson instead.

actually readConfigFile is more appropriate.

mhegazy · 2016-02-27T20:10:33Z

src/services/jsTyping.ts

+        const tsdJsonPath = ts.combinePaths(cachePath, "tsd.json");
+        const tsdJsonDict = tryParseJson(tsdJsonPath, host);
+        if (tsdJsonDict) {
+            for (const notFoundTypingName of notFoundTypingNames) {


why do we do this here? the host knows what files were not found. it can cache that and know not to search for them. we do not need to do this everytime.

- Adding ".json" file extension filter when retrieving json files from host and removoing filter - simplify isTypingEnabled check

billti · 2016-02-29T18:01:31Z

src/compiler/commandLineParser.ts

+            if (jsonTypingOptions) {
+                for (const id in jsonTypingOptions) {
+                    if (id === "enableAutoDiscovery") {
+                        if (typeof jsonTypingOptions[id] === "boolean") {


else push an error?

sounds good - we're doing it if the other options are invalid so makes sense here as well.

- push error for invalid enableAutoDiscovery option - adding interfaces for jsons - removing updateNotFoundTypings - node_modules normalize file names before using - adding safeListPath to discoverTypings

mhegazy · 2016-03-01T19:55:26Z

src/compiler/types.ts

+        enableAutoDiscovery?: boolean;
+        include?: string[];
+        exclude?: string[];
+        [option: string]: any;


string[] | boolean instead of any

- Remove all references to Tsd. Instead pass a map of package names to cached typing locations

These modules are already installed and are not expected to change

mhegazy · 2016-03-02T06:45:54Z

src/services/jsTyping.ts

+
+        if (!safeList) {
+            const result = readConfigFile(safeListPath, (path: string) => host.readFile(path));
+            if (result.config) { safeList = result.config; }


nit, new lines after {

mhegazy · 2016-03-02T06:49:47Z

LGTM

- Renaming DiscoverTypingsSettings to DiscoverTypingsInfo to match host

Salsa: JS support for discovering and acquiring d.ts files

mihailik · 2016-03-04T10:46:17Z

src/compiler/core.ts

@@ -278,6 +278,14 @@ namespace ts {
        return hasOwnProperty.call(map, key);
    }

+    export function getKeys<T>(map: Map<T>): string[] {
+        const keys: string[] = [];
+        for (const key in map) {


Why no hasOwnProperty check here?

It should have. Thanks for pointing out.

zhengbli · 2016-03-04T22:17:01Z

src/services/jsTyping.ts

+                safeList = result.config;
+            }
+            else {
+                safeList = {};


This is wrong. This will make an empty safeList, so nothing will pass the safety check. Should make it undefined instead.

undefined means you will keep looking for it the next time. that does not help if it still does not exist. if the intent is to let all things pass through if there is no safe list, then we should have a sentinel value to indicate its absence.

We should keep looking only if the safeListPath is provided. If it is not, we should stop trying. So this if block should have the condition of if (!safeList && safeListPath) With that I don't think we need a indicator value

so keep trying if a path was provided, but the file does not exist? what value would that give you, it is most probably not going to be there.

I was thinking that we were passing in a safeListPath only when we are confident about the existence of the file. But what you said makes sense, we should not try again if not found no matter what. I'll make that change soon.

jramsay added 2 commits February 21, 2016 21:35

Salsa: JS support for discovering and acquiring d.ts files

284d9f5

(Mostly isolating VS host changes from PR#6448)

Fixing lint issues caught by Travis CI build

0aaedc5

(Rules appear to be more strict - this was not caught on a local lint run)

RyanCavanaugh reviewed Feb 22, 2016
View reviewed changes

jramsay added 2 commits February 22, 2016 19:00

Addressing CR comments

5b06edb

- Adding check to ensure TypingOptions 'include' and 'exclude' arrays are composed of strings - Allow leading whitespace when removing comments from json

Merge branch 'master' into jsTypingForAcquireDts

317f5e2

billti reviewed Feb 23, 2016
View reviewed changes

jramsay added 4 commits February 22, 2016 19:33

Switch let -> const from lint validation

71bfefc

Adding devDependencies to the list of typings to merge

20511f8

Using removeComments from commandLineParser. This is more robust as i…

18883f9

…t removes both single and multiline comments

- renaming resolveTypeDefinitions to discoverTypings for consistency …

70ca4bd

…with jsTypings - simplifying typingOptions parsing after associated managed host changes

RyanCavanaugh reviewed Feb 26, 2016
View reviewed changes

zhengbli and others added 3 commits February 26, 2016 14:15

Unify the use of "filter", "map" and "Object.keys" functions

39a51d3

CR feedback

5981d8e

Adding optionalDependencies and peerDependencies to the list typings …

f76ef47

…to merge in if present.

mhegazy reviewed Feb 27, 2016
View reviewed changes

- Removing ts. from jsTyping.js

0346a98

- Adding ".json" file extension filter when retrieving json files from host and removoing filter - simplify isTypingEnabled check

billti reviewed Feb 29, 2016
View reviewed changes

- replacing TryParseJson with existing readConfig

b3ceea3

- push error for invalid enableAutoDiscovery option - adding interfaces for jsons - removing updateNotFoundTypings - node_modules normalize file names before using - adding safeListPath to discoverTypings

mhegazy reviewed Mar 1, 2016
View reviewed changes

jramsay added 2 commits March 1, 2016 18:52

- Adding DiscoverTypingsSettings

6aad783

- Remove all references to Tsd. Instead pass a map of package names to cached typing locations

- Removing filesToWatch from getTypingNamesFromNodeModuleFolder.

4bbdf2a

These modules are already installed and are not expected to change

mhegazy reviewed Mar 2, 2016
View reviewed changes

- Adding new lines after { for single-line if statements

e8772bc

- Renaming DiscoverTypingsSettings to DiscoverTypingsInfo to match host

jramsay added a commit that referenced this pull request Mar 2, 2016

Merge pull request #7179 from Microsoft/jsTypingForAcquireDts

de47fcc

Salsa: JS support for discovering and acquiring d.ts files

jramsay merged commit de47fcc into master Mar 2, 2016

mihailik reviewed Mar 4, 2016
View reviewed changes

mhegazy deleted the jsTypingForAcquireDts branch March 4, 2016 17:46

zhengbli reviewed Mar 4, 2016
View reviewed changes

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Salsa: JS support for discovering and acquiring d.ts files #7179

Salsa: JS support for discovering and acquiring d.ts files #7179

jramsay commented Feb 22, 2016

RyanCavanaugh Feb 22, 2016

jramsay Feb 23, 2016

billti Feb 23, 2016

jramsay Feb 23, 2016

blakeembrey Feb 23, 2016

jramsay Feb 23, 2016

blakeembrey Feb 23, 2016

jramsay Feb 26, 2016

RyanCavanaugh Feb 26, 2016

jramsay commented Feb 26, 2016

mhegazy Feb 27, 2016

mhegazy Feb 27, 2016

jramsay Mar 1, 2016

mhegazy Feb 27, 2016

billti Feb 29, 2016

jramsay Mar 1, 2016

mhegazy Mar 1, 2016

mhegazy Mar 2, 2016

jramsay Mar 2, 2016

mhegazy commented Mar 2, 2016

mihailik Mar 4, 2016

zhengbli Mar 4, 2016

zhengbli Mar 4, 2016

mhegazy Mar 4, 2016

zhengbli Mar 4, 2016

mhegazy Mar 4, 2016

zhengbli Mar 5, 2016

Salsa: JS support for discovering and acquiring d.ts files #7179

Salsa: JS support for discovering and acquiring d.ts files #7179

Conversation

jramsay commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jramsay commented Feb 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Mar 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment