Update: Open TS peerDependency, warn non-supported version (fixes #167)

[JamesHenry]

This would be one way to address #167

Still not sure how I feel about it, but thought I would stick a working example on a branch and look for feedback.

@soda0289 @eslint/eslint-team

[eslintbot]

Thanks for the pull request, @JamesHenry! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

• Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[soda0289]

I like this solution. It gives the user more information on what to expect.

For example typescript 2.3 has some breaking changes again. This time with how JSX is handled, need to spend some more time looking into. When typescript upgrades we will have to release a new version and a warning will help user understand why the parser is failing.

[corbinu]

This feels like the best solution to me also

[alberto]

Not sure that will work for non-final releases: http://blog.npmjs.org/post/115305091285/introducing-the-npm-semantic-version-calculator

[soda0289]

We currently use ~2.2.1 for the typescript devDependency. Which should only include versions between >=2.2.0 and < 2.3.0. According to the calculator, https://semver.npmjs.com/, it will only include version 2.2.1 and not the insider preview 2.2.2-insiders.20170302 or any of the development releases like 2.3.0-dev.20170318. I can't think of any other cases where this would fail.

[soda0289]

@JamesHenry
I think we may need better logic to get the globally installed version of typescript. If we require('typescript') that will only check the devDependency version, which would always match the package.json, not the globally installed one.

[soda0289]

The more I think about this I think it will be fine since devDepencies are not installed. It should work as expected.

[JamesHenry]

@soda0289 The reason why we need it as a peerDependency in the first place is that we require('typescript') and use it as a library. A global installation of TypeScript gives you access to the compiler's CLI, but that is not our use-case. The require will use whatever version the user installed in their project, the devDependency is only relevant for this project when it runs its tests.

I think it is fair to say require('typescript') will always be the appropriate "active" TypeScript version

[soda0289]

That makes sense. I was thinking that typescript was bundled with this parser but that is not the case and only happens during development.

[JamesHenry]

Ok I am going to clean this up and request review

[eslintbot]

LGTM

[SimenB]

any particular reason this is not using 5?

[soda0289]

Can't think of one. I looked at the commit log and seems they only dropped support for AMD modules and minifying the code in v5. Do you want to create a PR?

[SimenB]

Sure!

Interesting message on running npm install here, btw.

npm WARN The package semver is included as both a dev and production dependency.

[soda0289]

We don't need it in devDependencies anymore as it is used during runtime.

[SimenB]

#246