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

Enable search of local Typescript Language Service Plugins (fix #45856) #45858

Merged
merged 2 commits into from Mar 23, 2018

Conversation

Projects
None yet
3 participants
@killerDJO
Copy link
Contributor

killerDJO commented Mar 15, 2018

Proposed change for #45856. Added option to discover plugins in project-local node modules

@msftclas

This comment has been minimized.

Copy link

msftclas commented Mar 15, 2018

CLA assistant check
All CLA requirements met.

@killerDJO killerDJO force-pushed the killerDJO:master branch from d9a307e to 7ae3655 Mar 19, 2018

@mjbvz
Copy link
Contributor

mjbvz left a comment

Looking good. Just a few small changes

@@ -74,6 +76,7 @@ export class TypeScriptServiceConfiguration {
&& this.localTsdk === other.localTsdk
&& this.npmLocation === other.npmLocation
&& this.tsServerLogLevel === other.tsServerLogLevel
&& this.tsServerPluginPaths === other.tsServerPluginPaths

This comment has been minimized.

@mjbvz

mjbvz Mar 20, 2018

Contributor

Should be array contents comparison

@@ -106,6 +106,16 @@
"description": "%typescript.tsserver.log%",
"scope": "window"
},
"typescript.tsserver.pluginPaths": {

This comment has been minimized.

@mjbvz

mjbvz Mar 20, 2018

Contributor

Add "isExecutable": true here. This prevents this from ever being used as a workspace setting

This comment has been minimized.

@killerDJO

killerDJO Mar 20, 2018

Contributor

Why do you think it should be a user-only setting?
Specifying pluginPaths on workspace level can be quite useful, especially in conjunction with relative paths. In this case plugins configuration can be committed to a source control and shared between team members.
If this is this because of the security concerns, I don't believe this is a more dangerous option than ability to specify typescript.tsdk on a workspace-level.

This comment has been minimized.

@mjbvz

mjbvz Mar 20, 2018

Contributor

Yes it is for security reasons. Tsdk asks for user confirmation before executing any code in the workspace but that flow is complicated so I don't want to duplicate it here. Instead we should just limit these this to be a user only setting

@killerDJO killerDJO force-pushed the killerDJO:master branch from 7ae3655 to 5d5dff1 Mar 21, 2018

@killerDJO

This comment has been minimized.

Copy link
Contributor

killerDJO commented Mar 21, 2018

@mjbvz Changes have been made according to your comments.

@killerDJO killerDJO force-pushed the killerDJO:master branch 3 times, most recently from f5c8e12 to 9c43780 Mar 21, 2018

@killerDJO killerDJO force-pushed the killerDJO:master branch from 9c43780 to 27f0ac1 Mar 22, 2018

@mjbvz mjbvz merged commit d3cd393 into Microsoft:master Mar 23, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@mjbvz

This comment has been minimized.

Copy link
Contributor

mjbvz commented Mar 23, 2018

Thanks! On track for the March vscode release

@mjbvz mjbvz added this to the March 2018 milestone Mar 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment