Force tslint to use the same typescript version as the language service

[andyrooger]

This PR mocks require when loading tslint to ensure that the version of typescript it uses is the same as the version used by the language service to parse source files.

Fixes #58

Tested in VS 2017 and VS Code. Note that using npm run devtest won't work by itself until the package is published, until then you'll need to npm install mock-require in the test project manually.

[angelozerr]

Thanks @andyrooger for your PR. To be honnest with you, I have not the capability to validate your PR.

@egamma what do you think?

[andyrooger]

@angelozerr Sure. Let me know if there's anything you need me to do to help it along.

[egamma]

@mjbvz what do you think of this approach in comparison to your approach where you patched NODE_PATH https://github.com/Microsoft/vscode/issues/25769#issuecomment-339823893.

@andyrooger version mismatches between typescript and tslint are indeed an open issue of this plugin as you have found out the hard way. Mocking the require call is an interesting approach. I´m a bit reluctant to use mocking in production code. My preference would be to only mock when there is a version mismatch. Would this be possible to do?

First, I want to make sure I understand the approach:

mockRequire('typescript', modules.typescript);

This mocks the ´typescript´ module in the node module cache to return the typescript module used by the plugin. This ensures when a tslint rule imports the ´typescript´ module later in the execution, for example here, then the rule gets the mocked typescript module, which always returns the typescript version that is used by the plugin (which is the version that hosts the plugin).

const tslint = mockRequire.reRequire('tslint');

Why do you need to reRequire tslint, the tslint module is not mocked, so this has no effect?

[andyrooger]

@egamma

I´m a bit reluctant to use mocking in production code.

I agree. If there's a better way to do this (e.g. @mjbvz's NODE_PATH approach) I'm all for it.

My preference would be to only mock when there is a version mismatch. Would this be possible to do?

Certainly. I started with this approach, but removed it since I thought mocking with the same version of TS shouldn't cause any problems and would be simpler without the separate execution paths. I'm happy to add a check back in though.

Why do you need to reRequire tslint, the tslint module is not mocked, so this has no effect?

As I understand it, the cache for require is shared across other plugins, this lets my workaround in #58 work. The idea is that if another plugin had loaded tslint for any reason, it would already be cached, with the package.json version of typescript loaded in. In this case I'd need to reRequire to refresh the cache and get tslint loaded with the mocked VS version.

Looking at this again, I think this probably needs to be un-mocked after we load tslint too, so we don't affect other plugins that load later. However I'm not certain we can fully un-mock typescript without knowing all the sub-dependencies of tslint that also use typescript.

Would this all be better as a separate package which contains the workaround from the bug? It would at least be clearer that other packages and plugins will be affected.

[egamma]

@andyrooger given this discussion I suggest to add a setting ´mockTypeScriptVersion´ that enables the workaround and it is off by default. In this way there is no need for a separate package with the workaround. This also does not complicate the code with a version check. It is opt-in by the user.

[andyrooger]

@egamma I've added a mockTypeScriptVersion option. I also tried 'un-mocking' typescript after loading tslint, but have reverted the change since it didn't work for me.

[egamma]

👍 This is a good pragmatic solution and the documentation states the consequence of the setting.

@angelozerr any concerns?

[mjbvz]

@egamma Seems good to me. Will this allow us to remove the NODE_PATH workaround?

[angelozerr]

@angelozerr any concerns?

No, it looks good for me too.

Thanks @andyrooger for your contribution! Thanks @egamma for your review.

[angelozerr]

Will this allow us to remove the NODE_PATH workaround?

@mjbvz which NODE_PATH do you mean? Perhaps it's an issue with vscode?

[egamma]

@mjbvz

@egamma Seems good to me. Will this allow us to remove the NODE_PATH workaround?

Let´s keep the NODE_PATH workaround. The mock workaround is disabled by default and I have a slight preference to tweaking the NODE_PATH.

[egamma]

@angelozerr

@mjbvz which NODE_PATH do you mean? Perhaps it's an issue with vscode?

The NODE_PATH work around is done in the VS Code side. You could consider something similar for you Eclipse implementation.

[angelozerr]

Thanks @egamma for your feedback.

This issue is now integrate in 0.9.9 which was published. Thanks @andyrooger !