Skip to content

fix(typescript-estree): persisted parse and module none #1521

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
Closed
wants to merge 10 commits into from
Closed

fix(typescript-estree): persisted parse and module none #1521

wants to merge 10 commits into from

Conversation

armano2
Copy link
Collaborator

@armano2 armano2 commented Jan 25, 2020

This is alternative (more extensive) change to how converter generates AST, this change simply removes needs of parent propagation for conversion logic.

This change contains history from #1516, and its described in #1516 (comment)

fixes #1502

@armano2 armano2 added bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree labels Jan 25, 2020
@armano2 armano2 requested a review from bradzacher January 25, 2020 19:04
@armano2 armano2 self-assigned this Jan 25, 2020
@typescript-eslint

This comment has been minimized.

Sign in to view
@armano2 armano2 marked this pull request as ready for review January 25, 2020 19:38
@armano2 armano2 removed the request for review from bradzacher January 26, 2020 02:38
@bradzacher bradzacher mentioned this pull request Jan 26, 2020
Merged
bradzacher
bradzacher previously approved these changes Jan 26, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

I think this is a net positive.
It adds a small amount of complexity, but it means we can never mess it up by forgetting a getTypeChecker call

packages/typescript-estree/src/create-program/createDefaultProgram.ts
const compilerHost = ts.createCompilerHost(commandLine.options, true);
const compilerHost = ts.createCompilerHost(
commandLine.options,
/* setParentNodes */ true,
Copy link
Member

Choose a reason for hiding this comment

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

do we need to set this now?

Copy link
Collaborator Author

@armano2 armano2 Jan 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

should not be needed anymore

packages/typescript-estree/src/create-program/createIsolatedProgram.ts
@@ -46,7 +46,7 @@ function createIsolatedProgram(code: string, extra: Extra): ASTAndProgram {
filename,
code,
ts.ScriptTarget.Latest,
true,
/* setParentNodes */ true,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@codecov
Copy link

codecov bot commented Jan 26, 2020 •
edited
Loading

Codecov Report

Merging #1521 into master will decrease coverage by 0.07%.
The diff coverage is 97%.

@@            Coverage Diff             @@
##           master    #1521      +/-   ##
==========================================
- Coverage   95.57%   95.49%   -0.08%     
==========================================
  Files         149      150       +1     
  Lines        6685     6636      -49     
  Branches     1913     1878      -35     
==========================================
- Hits         6389     6337      -52     
  Misses        112      112              
- Partials      184      187       +3
Impacted Files Coverage Δ
packages/typescript-estree/src/convert.ts 99.43% <100%> (ø) ⬆️
.../eslint-plugin/src/util/explicitReturnTypeUtils.ts 100% <100%> (ø)
...-plugin/src/rules/explicit-function-return-type.ts 100% <100%> (ø) ⬆️
packages/eslint-plugin/src/util/astUtils.ts 84.37% <80%> (-3.13%) ⬇️
...plugin/src/rules/explicit-module-boundary-types.ts 85.45% <94.44%> (-8.52%) ⬇️

@armano2
Copy link
Collaborator Author

armano2 commented Jan 26, 2020

make sure to validate all lines if i'm passing correct property as parent (i know its huge)

@bradzacher
Copy link
Member

I eyeballed it all and it looked fine.
I'd hope that the tests would break if there was something you got wrong.
Seeing green so I think you got it all.

@bradzacher
Copy link
Member

TBF as well, there's only 6 instances of parent. in convert.ts.
So there's only a few cases to check.
Might be worth triple checking that there are tests covering these cases

@armano2 armano2 changed the title fix(typescript-estree): persisted parse and module none (version 2) fix(typescript-estree): persisted parse and module none Jan 30, 2020
@armano2
Copy link
Collaborator Author

armano2 commented Jan 30, 2020 •
edited
Loading

I did some additional testing, and it seems that we need parent nodes, than this change is not going to work,

.parent is mostly used by tsutils and if i turn off parent nodes its starting to fail

first place where it actually fails is comment generation as this relies on parents, i will recommend for now just go with #1516 or some variation of it (mayby we could limit number of calls by checking if first node has parent?)

@bradzacher
Copy link
Member

😢

That sucks. I didn't even think about the fact that tsutils uses parent.
Okay. reopen that and we can go with it instead.

@armano2
Copy link
Collaborator Author

armano2 commented Jan 30, 2020

than i'm closing this one as it will not work 😿

@armano2 armano2 closed this Jan 30, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
@armano2 armano2 deleted the remove-parent-calls branch February 7, 2021 08:34
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
bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parsing errors when tsconfig.json's compilerOption's module is set to none
2 participants
@armano2 @bradzacher