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

Breaking: Bump to [email protected], typescript@~3.2.1 #576

Closed
wants to merge 6 commits into from

Conversation

nevir
Copy link

@nevir nevir commented Dec 6, 2018

From what I can tell, the main change in typescript-estree is added support for BigIntLiterals. Added some snapshots for 'em, as well.


Should this be a major bump due to the typescript upgrade? Or only minor?

@jsf-clabot
Copy link

jsf-clabot commented Dec 6, 2018

CLA assistant check
All committers have signed the CLA.

@nevir nevir changed the title Bump to [email protected], typescript@~3.2.1 Update: Bump to [email protected], typescript@~3.2.1 Dec 6, 2018
@armano2
Copy link
Contributor

armano2 commented Dec 6, 2018

can you add new nodes to visitor-keys

@kaicataldo
Copy link
Member

Given the discussion in this PR, the team seems to be in general consensus that the "supported TS version" change is a breaking change.

@kaicataldo kaicataldo added breaking This change is backwards-incompatible and removed triage labels Dec 6, 2018
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind also updating the readme with the supported TS version range?

@nevir nevir changed the title Update: Bump to [email protected], typescript@~3.2.1 Breaking: Bump to [email protected], typescript@~3.2.1 Dec 7, 2018
@nevir
Copy link
Author

nevir commented Dec 7, 2018

can you add new nodes to visitor-keys

Hmm, I'm not quite sure which nodes would need to be added to visitor-keys (and by that, you mean ./visitor-keys.js?). Seems like this is only adding a new literal value type (BigIntLiteral ), and that doesn't seem to fit with the structure of that file. I admit, though, I'm not too familiar w/ how eslint juggles the AST.

Do you mind also updating the readme with the supported TS version range?

👍 Done. Also switched the version constraint to a ^, as suggested in #573 (comment)

@armano2
Copy link
Contributor

armano2 commented Dec 7, 2018

@nevir you should just add "BigIntLiteral": [] to ./visitor-keys.js

@platinumazure
Copy link
Member

Is BigIntLiteral a node type? Or is it a type of the existing Literal node?

@armano2
Copy link
Contributor

armano2 commented Dec 7, 2018

@justinanastos
Copy link

I just wanted to chime in and say thank you to everyone who's working on this! Great work!

@nevir
Copy link
Author

nevir commented Dec 11, 2018

Added the node type

@nevir
Copy link
Author

nevir commented Dec 14, 2018

@kaicataldo, is there anything else you'd like me to tweak?

@@ -46,7 +46,7 @@
"dependencies": {
"eslint-scope": "^4.0.0",
"eslint-visitor-keys": "^1.0.0",
"typescript-estree": "5.3.0"
"typescript-estree": "^6.0.0"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7.0.0 just came out

Copy link
Contributor

@armano2 armano2 Dec 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7.0.0 has breaking changes -> ast structure has changed
JamesHenry/typescript-estree@159c325

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KaelWD i did pr with changes for 7.0.0 in #584

@platinumazure
Copy link
Member

Hi @nevir, apologies for letting this sit over the holidays.

Is this PR still needed now that #589 has been created?

@JamesHenry
Copy link
Member

Thanks a lot for this @nevir and I am sorry for the delay - your commits (with you still as the author) are included directly in #596, so I am closing this in favour of that one.

Thanks again!

@JamesHenry JamesHenry closed this Jan 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking This change is backwards-incompatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants