angelozerr / tslint-language-service

TypeScript 2.2.1 plugin for tslint
MIT License
188 stars 22 forks source link

Allow usage by tsserver 2.2.1 #7 #8

Closed Istar-Eldritch closed 7 years ago

Istar-Eldritch commented 7 years ago

Issue #7 This PR modifies the index.ts so it complains both with the PluginModuleFactory and PluginModule interfaces. This allows for the tsserver 2.2.1 to load the plugin and keeps retro compatibility with previous loaders of this library.

This PR also modifies the project structure in a way that can be interpreted as opinionated. This are the modifications:

The reason for the changes is that the tests also need to be compiled and would pollute the release files.

Also, I believe this release files / artifacts shouldn't be part of the source code. These should be generated right before release. This avoids conflicts in merges etc on files no developer really cares about. The release of this files should be done probably using an npm module.

I also modified the package.json file:

angelozerr commented 7 years ago

@Istar-Eldritch thanks for your PR. For the removed compiled js files, I have tried to follow the same idea than TypeScript which have a lib folder and which contains compiled js files. So why could not do that?

Why do you use tape (I didn't know) and not mocha, chai like TypeScript?

angelozerr commented 7 years ago

For compiled js files, it seems angular language service doesn't host js files https://github.com/angular/angular/tree/master/packages/language-service

I'm OK with that. It's just the .build folder which seems strange (I have never seen that). I think we should activate test too with travis.

Istar-Eldritch commented 7 years ago

I'm glad you found other repos that do not keep the deployment files. Having the compiled files in the repo sounds to me like committing the java artifacts or classes. It doesn't really add value and it may in fact create problems with merges and conflicts, appart from deviating the attention during CR from the changes that really matter in the project.

Regarding the reasons behind I started with tape and not mocha, this blog post summarizes it quite well.

At @repositive we switched from mocha to tape 6 months ago for all our backend services, and we won't look back. The reason we switched was the shared state of the tests, at that moment I was doing some crazy test with streams, and the simplicity of the structured vs stateful tests helped a lot.

As I told you, I'm used to do stuff with mocha, so if you see benefits on using it I can change the PR.

angelozerr commented 7 years ago

Thanks a lot @Istar-Eldritch for your great claraification. I'm not a big expert with JS tests framworks, so I trust you.

I think next step is to execute test with travis.