Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

Breaking: typescript-estree to 7.0.0 and typescript to 3.2.1 #584

Closed
wants to merge 8 commits into from

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Dec 24, 2018

This PR contains changes from #576 and deprecates changes from #581

recently i started fixing issues in typescript-estree (most of changes are improvements to typing and tests)

but there was important changes to generated AST:


  • There was change of one node: DeclareFunction got renamed to TSDeclareFunction to remove confuction.
  • Alot of fields got update and we have now optional declare property to determine if node is "declaration". TSDeclareFunction has declare property always set to true.
  • VariableDeclarator[kind='type'] was causing a lot of issues within esling rules and it was extracted to its own type: TSTypeAliasDeclaration
  • modifier abstract is now optional (true or undefined)

closes: #581, #576
fixes: #414, #588, #384

@armano2 armano2 force-pushed the estree-7.0 branch 2 times, most recently from f396a95 to ebd229d Compare December 24, 2018 23:02
@armano2 armano2 changed the title Breaking: Bump to typescript-estree@7.0.0, typescript@~3.2.1 Bump to typescript-estree@7.0.0, typescript@~3.2.1 Dec 24, 2018
@armano2 armano2 changed the title Bump to typescript-estree@7.0.0, typescript@~3.2.1 Upgrade: typescript-estree to 7.0.0 Dec 24, 2018
@armano2 armano2 changed the title Upgrade: typescript-estree to 7.0.0 Upgrade: typescript-estree to 7.0.0 and typescript to 3.2.1 Dec 24, 2018
@platinumazure
Copy link
Member

I'll try to review this tomorrow. Sorry for the delay!

@platinumazure
Copy link
Member

Hi @armano2, should this be closed now that #589 has been created?

@armano2
Copy link
Contributor Author

armano2 commented Dec 28, 2018

depends how you want to release it, if you prefer doing smaller patches, you can release them one by one. and i will rebase changes.

@platinumazure
Copy link
Member

platinumazure commented Dec 28, 2018

@armano2 I don't see much value in releasing with typescript-estree 7.x unless there is a good chance that there are people in the community who are using typescript-estree 7.x for their projects, are currently blocked from using ESLint since we (typescript-eslint-parser) are still on a 5.x version, and would also be blocked from upgrading if we went straight to 8.x. If there isn't that explicit need to provide some support for 7.x, I would rather just jump straight to 8.x since it's a bit less work on the maintenance side.

@kaicataldo @mysticatea What do you think of the above? Should we cut a major release of typescript-eslint-parser with typescript-estree 7.x support "just in case", or can we jump to 8.x? And if we want to support 7.x as well, should we do a release with 6.x before that?

@JamesHenry
Copy link
Member

@platinumazure I'm sorry for the noise - it's purely down to this project and typescript-estree having very different approaches to releases.

I wanted typescript-estree to be a low touch as possible - there is a strict following of semver, and semantic-release handles everything automatically upon merge to master.

I would strongly suggest the "major" version of TypeScript is used as the source of truth (TypeScript does not follow semver, so this is not all that intuitive, but major in terms of TypeScript effectively refers to the second digit. E.g. the parser would move from supporting X.1 to X.2)

Please let me know if you want to discuss it any further.

For these PRs specifically, I see no reason not to jump straight to the latest typescript-estree, as the version of TypeScript which is targeted/supported has not changed. It's just that we have been refining the AST structure a lot recently, and all changes are released as individual major version bumps.

I am doing a similar (on the face of it) large jump in Prettier right now: prettier/prettier#5728 - as you can see the amount of changes needed are not as intense as you might have initially expected from v6 -> v13

@kaicataldo
Copy link
Member

As far as upgrading typescript-eslint goes, I like the idea of tracking the supported version of TypeScript (following the version of semver that TS follows as described by @JamesHenry).

@JamesHenry
Copy link
Member

Closing in favour of #596

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

type declarations should have distinct AST from vars
5 participants