Loading issue when bundling tslint language server plugin as a VS Code extension

[egamma]

@kieferrm @mjbvz I've created a VS Code extension (vscode-ts-lint) for the tslint-language-service using the typescriptServerPlugins contribution point. See the package.json included below.

However, there is an issue with how the tslint module can load the typescript module it requires. To describe the issue, let me first explain the setup, when the language server plugin is installed into a workspace using npm install tslint typescript tslint-language-service. This results in the following module layout inside the node_modules folder.

node_modules
   tslint
   tslint-language-service
   typescript

Notice that the typescript module is a peer to the tslint module and the tslint module can require the typescript module. This is the same typescript module that is used by vscode's typescript server.

Now, the question is what dependencies should be defined for the vscode tslint extension? To make the extension work I had to include typescript as a dependency otherwise tslint cannot find the typescript module and the tslint-language-service plugin does not load. Since the existing tslint rules import typescript they need to get at a typescript module. This is the error message when the typescript module is not bundled.

Info 11   Loading tslint-language-service from c:\Program Files (x86)\Microsoft VS Code Insiders\resources\app\extensions\node_modules\typescript\lib\tsserver.js/../../.. (resolved to c:/Program Files (x86)/Microsoft VS Code Insiders/resources/app/extensions/node_modules/node_modules)
Info 12   Failed to load module: {}
Info 13   Loading tslint-language-service from C:\Users\egamma\.vscode-insiders\extensions\eg2.vscode-ts-tslint-0.0.1 (resolved to C:/Users/egamma/.vscode-insiders/extensions/eg2.vscode-ts-tslint-0.0.1/node_modules)
Info 14   Failed to load module: {"code":"MODULE_NOT_FOUND"}
Info 15   Couldn't find tslint-language-service anywhere in paths: c:\Program Files (x86)\Microsoft VS Code Insiders\resources\app\extensions\node_modules\typescript\lib\tsserver.js/../../..,C:\Users\egamma\.vscode-insiders\extensions\eg2.vscode-ts-tslint-0.0.1

However, the tslint module should use the same typescript module that is used by the vscode typescript server and not include yet another one. This results in loading two versions (!) of the typescript module into the language server.

One idea would be that the TS language server tweaks the node require path, so that the typescript module used by the language server is found when a tslint rule requires typescript.

package.json

{
    "name": "vscode-ts-tslint",
    "displayName": "vscode-ts-tslint",
    "description": "tslint integration as a TS server plugin",
    "version": "0.0.1",
    "publisher": "eg2",
    "engines": {
        "vscode": "^1.11.0"
    },
    "categories": [
        "Other"
    ],
    "main": "./out/src/extension",
    "contributes": {
        "typescriptServerPlugins": [
            "tslint-language-service"
        ]
    },
    "scripts": {
        "vscode:prepublish": "tsc -p ./",
        "compile": "tsc -watch -p ./",
        "postinstall": "node ./node_modules/vscode/bin/install",
        "test": "node ./node_modules/vscode/bin/test"
    },
    "devDependencies": {
        "typescript": "^2.0.3",
        "vscode": "^1.0.0",
        "mocha": "^2.3.3",
        "@types/node": "^6.0.40",
        "@types/mocha": "^2.2.32"
    },
    "dependencies": {
        "tslint-language-service": "^0.9.2",
        "tslint": "^5.1.0",
        "typescript": "^2.3.1"
    },
    "extensionDependencies": [
        "vscode.typescript"
    ]
}

[mjbvz]

To further complicate matters, in this case you can't actually change the tslint code, correct? It has to remain require('typescript')?

Since the plugin is going to be run by TypeScript and not the extension, we may be able to set NODE_PATH when starting the typescript server. I'm not sure what the possible impact of this would be however

[RyanCavanaugh]

Extensions shouldn't be calling require("typescript"). The extension needs to accept the passed-in ts object in its top-level factory function and use that instead, otherwise they will fail badly in version mismatch scenarios. TSLint is properly architected to accept this object already.

[RyanCavanaugh]

See also https://github.com/RyanCavanaugh/sample-ts-plugin/blob/master/lib/index.ts#L3

[egamma]

TSLint is properly architected to accept this object already.

@RyanCavanaugh do you have a pointer how to pass the passed-in ts object on to tslint? I fully agree that this is the right way. However, the tslint rules are all using `require("typescript").

https://github.com/palantir/tslint/blob/master/src/rules/alignRule.ts#L19
https://github.com/palantir/tslint/blob/master/src/rules/noUnusedVariableRule.ts#L19

[mjbvz]

@egamma I added a workaround to set NODE_PATH so that typescript should resolve to the vscode version of typescript. I just tested this with the tslint language service plugin extension by deleting typescript from its node_modules.

Can you please let me know if this solves this issue or let me know if there are additional steps I can run through to test this

[egamma]

@mjbvz this is good!

An additional test you can make is whether this change also enables to eliminate the typescript dependency from the extension that bundles the tslint language service plugin. This is the dependency https://github.com/Microsoft/vscode-ts-tslint/blob/c33c12ed1f1c5a3d29d52c0b5c2b784353f5bc4e/package.json#L45. From what you describe this should be the case.

One concern I have is that tweaking the NODE_PATH dynamically not really a recommended practice.

[mjbvz]

Yes, setting node_path seems to allow that in my testing.

@RyanCavanaugh I agree with Erich that setting node_path on the VS Code side is not great. Could we be doing something similar on the TS Server side instead (perhaps handing plugins a mocked out version of require that resolves typescript properly)?

[RyanCavanaugh]

I don't see how we could do that. When node loads the module we have no ability to mess with require.

Plugins just need to do the right thing. We shouldn't try to get too tricky trying to work around their bugs.

[mjbvz]

I think @egamma's point is that this is not always something the extension has control over. The tslint plugin can do the right thing but it still depends on tslint which does not.

As for changing the behavior of require, we do something similar for vscode extensions: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/node/extHost.api.impl.ts#L662

Another approach would be to run the extension inside a sandbox that we hand a specialized version of require

[mjbvz]

Closing since we have addressed this issue on the vs code side. @RyanCavanaugh I'll leave it up to you to decide if any action is needed on the TypeScript side