New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(*): change TypeScript version range to >=3.2.1 <3.4.0 #184

Merged
merged 4 commits into from Feb 4, 2019

Conversation

Projects
None yet
7 participants
@JamesHenry
Copy link
Member

JamesHenry commented Feb 1, 2019

@typescript-eslint/core-team I always use to do these TypeScript version changes as major version bumps for clarity - regardless of whether or not breaking changes were required.

It seems for 3.3, we do not need any changes.

Do we want to do major bumps only when breaking changes are required? Or also force them for TS version changes?

Bear in mind, a standard user will receive a warning in their console that they are using an unsupported version if they switch between the versions before and after this PR without updating their TypeScript version.

@j-f1

This comment has been minimized.

Copy link
Member

j-f1 commented Feb 2, 2019

IMO minor versions can add warnings but not errors.

@bradzacher
Copy link
Member

bradzacher left a comment

I don't think it's worth a major bump if it doesn't break anything.
It will just make our version number grow too quickly considering TS is looking to release a new version every month or so.
Which means we'll be back to v20 in no time

@aboyton

This comment has been minimized.

Copy link
Contributor

aboyton commented Feb 4, 2019

In my experience it's not that uncommon for your code to not build with a bump in TypeScript version as it finds some new type bugs in your code. I'm assuming that when you bump the version of TypeScript in this library then my code won't be able to be linted?

That said, I'm not sure if we need to do a major bump even if technically this is a breaking change (TypeScript doesn't). I think it makes the number bigger than it needs to be.

@armano2

This comment has been minimized.

Copy link
Member

armano2 commented Feb 4, 2019

@aboyton, we are not installing typescript and we are allowing to use *, but we are only testing against one version and if yours is not same as defined in package you are going to see warning in console.

https://github.com/typescript-eslint/typescript-eslint#supported-typescript-version

@aboyton

This comment has been minimized.

Copy link
Contributor

aboyton commented Feb 4, 2019

@aboyton, we are not installing typescript and we are allowing to use *, but we are only testing against one version and if yours is not same as defined in package you are going to see warning in console.

https://github.com/typescript-eslint/typescript-eslint#supported-typescript-version

Sounds great. Ignore me then. Personally I try to keep on the tip as much as I can and so as long as you do too I don't really mind how you name things. Thanks everyone again for all of the work on this. Now to bump to 3.2 to I can start using this :)

@Jessidhia

This comment has been minimized.

Copy link
Contributor

Jessidhia commented Feb 4, 2019

It would be nice to be able to silence the warning with an environment variable or something to that effect, though. Running eslint in parallel on a monorepo prints a big warning banner for every single package that is checked; same if you have multiple instances of eslint-loader (one warning for each instance, you'll likely have at least 3 instances if using thread-loader).

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Feb 4, 2019

If no change, can we support both versions? For example,

-     "typescript": "~3.2.1"
+     "typescript": ">=3.2.1 <3.4.0"
@aboyton

This comment has been minimized.

Copy link
Contributor

aboyton commented Feb 4, 2019

From memory there’s a way to turn the warning off. Can we document that as I’ve had to do it in the past as I had tools that required ESLint to output nothing when nothing was wrong? In the past I’ve wanted to update TypeScript versions faster than this repo has (and so silenced the warning).

JamesHenry added some commits Feb 4, 2019

@JamesHenry JamesHenry dismissed stale reviews from bradzacher and armano2 via f82553d Feb 4, 2019

@JamesHenry JamesHenry changed the title feat(*): bump TypeScript version to ~3.3.1 feat(*): change TypeScript version range to >=3.2.1 <3.4.0 Feb 4, 2019

@JamesHenry

This comment has been minimized.

Copy link
Member Author

JamesHenry commented Feb 4, 2019

I have implemented all the feedback.

  • There is now a supported range of >=3.2.1 <3.4.0
  • Better docs
  • New documented warnOnUnsupportedTypeScriptVersion parserOption for parser which is true by default to match the current behaviour
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 4, 2019

Codecov Report

Merging #184 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #184      +/-   ##
=========================================
+ Coverage    96.2%   96.2%   +<.01%     
=========================================
  Files          51      51              
  Lines        2449    2451       +2     
  Branches      368     369       +1     
=========================================
+ Hits         2356    2358       +2     
  Misses         48      48              
  Partials       45      45
Impacted Files Coverage Δ
packages/parser/src/parser.ts 100% <100%> (ø) ⬆️
packages/typescript-estree/src/parser.ts 91.22% <100%> (-0.08%) ⬇️
Show resolved Hide resolved README.md
@armano2

armano2 approved these changes Feb 4, 2019

@JamesHenry JamesHenry merged commit f513a14 into master Feb 4, 2019

3 checks passed

Semantic Pull Request ready to be squashed
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 96.2% (+<.01%) compared to e37a1ed
Details

@JamesHenry JamesHenry deleted the ts-3.3 branch Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment