Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(typescript-estree): align optional fields #1429

Merged
merged 8 commits into from
May 10, 2020
Merged

feat(typescript-estree): align optional fields #1429

merged 8 commits into from
May 10, 2020

Conversation

armano2
Copy link
Member

@armano2 armano2 commented Jan 11, 2020 •
edited
Loading

This is BREAKING CHANGE

Goal of this change is to align how we are marking optional properties and methods,
Difference between this PR and #1100 is that instead of trying to add "optional" to key of method/property we are directly marking method with it.

This is more aligned with how typescript produces ast and how babel generates estree for this field.

read more: #1100 (comment)
context prettier/prettier#6601, prettier/prettier#6678

closes #1100
fixes #1097

cc @sosukesuzuki @thorn0 @fisker

@armano2 armano2 added bug Something isn't working enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree labels Jan 11, 2020
@armano2 armano2 added this to the 3.0.0 milestone Jan 11, 2020
@armano2 armano2 requested a review from bradzacher January 11, 2020 18:15
@armano2 armano2 self-assigned this Jan 11, 2020
@typescript-eslint

This comment has been minimized.

Sign in to view
@armano2 armano2 added breaking change This change will require a new major version to be released and removed enhancement New feature or request labels Jan 11, 2020
@armano2 armano2 marked this pull request as ready for review January 11, 2020 18:53
@bradzacher
Copy link
Member

why is this breaking? It looks like a backwards-compatible addition.

@armano2
Copy link
Member Author

armano2 commented Jan 13, 2020 •
edited
Loading

AST produced by our converter is going to differ for existing code base, this is not an issue for our plugin, but its a breaking change for eg. prettier

@bradzacher
Copy link
Member

As long as a consumer's runtime code doesn't break as part of the addition, it's non-breaking.
In this instance:

  • the fields are optional
  • no fields are being removed

So the overall shape of the AST is consistent, and existing code will not break.
Breaking snapshot tests is not included in consideration for breaking changes for obvious reasons (or else every new TS feature would require a major bump).

@codecov
Copy link

codecov bot commented Jan 17, 2020 •
edited
Loading

Codecov Report

Merging #1429 into v3 will decrease coverage by 0.00%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##               v3    #1429      +/-   ##
==========================================
- Coverage   93.78%   93.78%   -0.01%     
==========================================
  Files         172      172              
  Lines        7809     7808       -1     
  Branches     2243     2242       -1     
==========================================
- Hits         7324     7323       -1     
  Misses        227      227              
  Partials      258      258
Flag Coverage Δ
#unittest 93.78% <40.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
...mental-utils/src/eslint-utils/getParserServices.ts 18.18% <0.00%> (ø)
...-estree/src/create-program/createProjectProgram.ts 92.68% <ø> (ø)
...ges/typescript-estree/src/create-program/shared.ts 88.57% <ø> (ø)
...nt-plugin/src/rules/no-unused-vars-experimental.ts 91.48% <100.00%> (ø)
packages/typescript-estree/src/ast-converter.ts 100.00% <100.00%> (ø)
packages/typescript-estree/src/convert.ts 99.15% <100.00%> (-0.01%) ⬇️

@thorn0
Copy link
Contributor

thorn0 commented Jan 17, 2020

why is this breaking? It looks like a backwards-compatible addition.

Now, in a MethodDefinition node, the key is marked with optional: true (only if the key is an identifier). After this change it won't be marked.

@bradzacher
Copy link
Member

Ah, sorry - I missed the missing .key in result.optional.

Question:
Did we want to do both in the interim?
I imagine it'll be a bit clunky if prettier has to support both versions.
Could ease the transition if we kept the existing functionality in for a major version.

@thorn0
Copy link
Contributor

thorn0 commented Jan 17, 2020 •
edited
Loading

Don't worry about Prettier. It's already smart enough to find optional on the method definition node as class and interface methods are printed by the same code. And the AST is already structured this way for interfaces.

@armano2
Copy link
Member Author

armano2 commented Jan 17, 2020 •
edited
Loading

@bradzacher i will prefer to not replicate / duplicate informations across nodes, thats why its a breaking change.
prettier team (namely @thorn0) started implementing alternative ts parser (babel) and they have to support this change anyway as babel produces ast in this way.

95a6d4e#diff-91e3f2b1dd09dcb0133b1fd29dbb790aL363-L370

@bradzacher bradzacher changed the base branch from master to v3 May 10, 2020 00:23
bradzacher added 3 commits May 9, 2020 17:42
@bradzacher
Merge branch 'v3' into pr/armano2/1429
e1d0f9e 
# Conflicts:
#	packages/typescript-estree/tests/lib/__snapshots__/typescript.ts.snap
@bradzacher
Merge branch 'v3' into 1097-align-optional-field-for-methods
6ce8ee8
@bradzacher
test: snaps
048a133
@bradzacher bradzacher merged commit e1bf4a9 into typescript-eslint:v3 May 10, 2020
bradzacher pushed a commit that referenced this pull request May 11, 2020
@armano2 @bradzacher
feat(typescript-estree): align optional fields (#1429)
17cc82b
bradzacher pushed a commit that referenced this pull request May 14, 2020
@armano2 @bradzacher
feat(typescript-estree): align optional fields (#1429)
ac7f94f
bradzacher pushed a commit that referenced this pull request May 17, 2020
@armano2 @bradzacher
feat(typescript-estree): align optional fields (#1429)
cdf4b3d
bradzacher pushed a commit that referenced this pull request May 21, 2020
@armano2 @bradzacher
feat(typescript-estree): align optional fields (#1429)
0e0010f
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2020
@armano2 armano2 deleted the 1097-align-optional-field-for-methods branch February 13, 2021 15:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher left review comments

Assignees

@armano2 armano2

Labels
breaking change This change will require a new major version to be released bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

Missing optional when the key is MemberExpression in a computed optional methods.
3 participants
@armano2 @bradzacher @thorn0