Chore: Upgrade deps to eslint 4 and typescript-parser 4

[corbinu]

This should land after #30 I updated the deps. Cleaned up the linting to fit eslint-config-eslint 4 and then fixed the broken tests. I did test against both eslint 3 and 4. We should be ready to add new rules now!

[corbinu]

Spoke to soon, but looks like it is actually the test harness @JamesHenry do you have any idea what might have broken it?

[soda0289]

I can take a look. A lot of re-factoring was done to the Linter class in eslint don't think too much changed with RuleTester.

[soda0289]

@corbinu

If you change the rule tester line:
ruleTester.run("no-unused-vars", ruleNoUnusedVars, {
to
ruleTester.run("ts/no-unused-vars", ruleNoUnusedVars, {

It runs the tests with errors. Not sure why that change needs to happen. Still looking into it.

[JamesHenry]

Thanks a lot @corbinu! As mentioned on the other PR, we will now not need to change the name, so please remove the relevant changes from the PR whenever you get chance to fix up the ruleTester based on @soda0289 suggestion

[corbinu]

@JamesHenry @soda0289 Thanks I just rolled it back let me figure out what broke the no-unused and then will squash it down and we are good to go

[corbinu]

@soda0289 That change seems to be causing only the main no-unused-vars to run as when I try console.log in typescript/no-unused-vars to see why the decorators aren't getting marked I never get any output

[soda0289]

@corbinu I'm not sure what the test was supposed to be checking for. I guess it wants both rules to be run at the same time.

This line:
ruleTester.run("typescript/no-unused-vars", ruleNoUnusedVars,

Is loading the original rule since: ruleNoUnusedVars is the original no-unused-vars rule. Maybe we should change that part as well. Can spend some more time tonight digging into it,

[corbinu]

@soda0289 Ok I think I know whats happening the change is causing eslint to overwrite typescript/no-unused-vars as the original rule. We need to change it back to no-unused-vars and then figure out why eslint isn't finding that rule even though it is defined on line 16 with this

eslint.defineRule("typescript/no-unused-vars", rule);

[soda0289]

@corbinu I think what might be happening is that RuleTester is creating its own Linter instance and eslint has a different Linter instance. In eslint v4 Linter is no longer a singleton meaning multiple instances can exists.

Instead of doing eslint.defineRule() try ruleTester.linter.defineRule()

[corbinu]

@soda0289 Haha just did that and was about to comment I found it. Thanks for all your help!

[corbinu]

@soda0289 @JamesHenry All set and ready for release! I bumped the version to 0.3.0 since we did release a 0.2.0 under my scope before.

[corbinu]

Also fixed bugs against both parser master and ts-2.4

[soda0289]

Theres a breaking change in the parser that will cause the type-annotation spacing rule to break. Not sure if we should fix that now or after we release a new version of the parser.

eslint/typescript-eslint-parser#319

[corbinu]

@soda0289 I don't think there is a way to do it without breaking support for version 3 of the parser so lets merge this and then we can release a version 0.4.0 that supports version 4 of the parser and typescript 2.4

[JamesHenry]

Hey @corbinu, thanks for this, it is nearly ready to merge, the only thing I would ask is that you please remove the package-lock.json file and add a .npmrc file to prevent it being created in the future. This is purely to mirror the position that we took for the ESLint repo.

See:

eslint/eslint#8742
eslint/eslint#8747

[corbinu]

@JamesHenry Now that the breaking change is in parser 4.0.0 I do think I should fix that first.

Also I am really sad to hear that but will do for parity.

[corbinu]

@JamesHenry Wouldn't it make more sense to add package-lock.json to .gitignore that way at least reinstalls during development would be faster?

It seems like eslint/eslint#8742 misunderstands a bit what the package-lock does and doesn't do :/

[soda0289]

@corbinu Based on this comment: eslint/eslint#8742 (comment)
even if it is added to .gitignore it will still be included in the commit when npm version is called.

[corbinu]

@soda0289 Yes but my understanding is that specifically has to do with eslint's release system which we aren't using here. Also package-lock for dependencies is not respected on npm install so it ending up in the release or not does absolutely nothing. It only affects when somebody git clones the repo.

So what we are talking about is A) if it is committed then when any of us forks this repo we are all working of the same dep tree B) it is not committed because it is in the .gitignore at least we each get a faster npm install on subsequent installs in our own versions.

Also this comment

I think we should not ignore `package-lock.json`. If `package-lock.json` exists, npm seems to ignore `package.json`. As a result, I guess local `package-lock.json` will cause confusing if `git pull` changes `package.json`.

Is no longer true.

[soda0289]

@corbinu I see your point. I'm not too familiar on the details of npm and package-lock.json just thought I'd share the reason why eslint did not use .gitignore

[corbinu]

@soda0289 I just wish I had seen that issue before it was merged would have brought all this up there. However if you guys want it to match I understand

[corbinu]

@JamesHenry All set for release but I still don't like it ;)

[JamesHenry]

Thanks! Published as eslint-plugin-typescript@0.3.0