Skip to content

feat(typescript-estree): add strict type mapping to parserServices.esTreeNodeToTSNodeMap#1382

Merged
bradzacher merged 11 commits intotypescript-eslint:masterfrom
armano2:esTreeNodeToTSNodeMap
Jan 9, 2020
Merged

feat(typescript-estree): add strict type mapping to parserServices.esTreeNodeToTSNodeMap#1382
bradzacher merged 11 commits intotypescript-eslint:masterfrom
armano2:esTreeNodeToTSNodeMap

Conversation

@armano2
Copy link
Copy Markdown
Collaborator

@armano2 armano2 commented Dec 26, 2019 •
edited
Loading

I'm unsure if we actually want this change and if place where type is defined is good one.

i was also thinking about name: ParserServicesConvertibleTypes

rules that will have to be updated to use more strict types instead of Node:

  • no-unnecessary-condition
  • no-unnecessary-qualifier
  • no-unused-vars-experimental
  • prefer-readonly
  • prefer-regexp-exec
  • prefer-string-starts-ends-with
  • promise-function-async
  • restrict-plus-operands
  • restrict-template-expressions
  • strict-boolean-expressions

@bradzacher what do you think about this?

for example in prefer-regexp-exec function isStringType is used to check type but as argument it accepts TSESTree.Node and it should TSESTree.LeftHandSideExpression

@typescript-eslint

This comment has been minimized.

Sign in to view
@armano2 armano2 marked this pull request as ready for review December 26, 2019 04:43
@bradzacher
Copy link
Copy Markdown
Member

bradzacher commented Dec 26, 2019 •
edited
Loading

there are a lot of places that I just dropped in TSESTree.Node when it doesn't actually matter what the node type is (no explicit checks on props, etc).

Instead of using an excludes, I think it would be a good idea to strictly type the get method on ParserWeakMap - we know the mapping that exists because we own the conversion logic, so we should be able to do something similar to RuleListener.

Also for Program:exit, could you just add that to the RuleListener list? It's a pretty common pattern.

For the getter on ParserWeakMap, and for RuleListener, we should probably just create a build script to generate the files.

WDYT?

@bradzacher bradzacher changed the title fix(typescript-eslint): correct types of service node maps fix: correct types of service node maps Dec 26, 2019
@bradzacher bradzacher added the bug Something isn't working label Dec 26, 2019
@armano2 armano2 added the DO NOT MERGE PRs which should not be merged yet label Dec 26, 2019
@armano2
Copy link
Copy Markdown
Collaborator Author

armano2 commented Jan 6, 2020

@bradzacher type mapping is added, i'm unsure if we should expose it to plugins

bradzacher
bradzacher requested changes Jan 7, 2020
View reviewed changes
Copy link
Copy Markdown
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 was actually thinking we should do something like this, but I was too lazy to go through the parser logic and pull out the mapping. So thanks!

I think this is a good idea, and I like the idea of removing the manual conversion as I wouldn't be surprised if we got it wrong around the place.

Could you also run the no-unnecessary-type-arguments rule to fix up any manual annotations that are now unnecessary?

Comment thread packages/typescript-estree/src/ts-estree/estree-to-ts-node-types.ts
Comment thread packages/typescript-estree/src/convert.ts Outdated
Comment thread packages/typescript-estree/src/parser-options.ts Outdated
Comment thread packages/experimental-utils/src/index.ts Outdated
armano2
armano2 commented Jan 9, 2020
View reviewed changes
Comment thread packages/experimental-utils/package.json
bradzacher
bradzacher reviewed Jan 9, 2020
View reviewed changes
Comment thread packages/typescript-estree/src/ts-estree/estree-to-ts-node-types.ts
armano2
armano2 commented Jan 9, 2020
View reviewed changes
Comment thread packages/typescript-estree/src/convert.ts
armano2
armano2 commented Jan 9, 2020
View reviewed changes
Comment thread packages/typescript-estree/src/ts-estree/estree-to-ts-node-types.ts
bradzacher
bradzacher approved these changes Jan 9, 2020
View reviewed changes
Copy link
Copy Markdown
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.

LGTM - one question then I'm happy for this to merge.

Comment thread packages/typescript-estree/src/ts-estree/estree-to-ts-node-types.ts
@armano2 armano2 removed the DO NOT MERGE PRs which should not be merged yet label Jan 9, 2020
@armano2 armano2 added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin package: typescript-estree Issues related to @typescript-eslint/typescript-estree labels Jan 9, 2020
@bradzacher bradzacher changed the title fix: correct types of service node maps feat(typescript-estree): add strict type mapping to parserServices.esTreeNodeToTSNodeMap Jan 9, 2020
@bradzacher bradzacher added enhancement New feature or request and removed bug Something isn't working labels Jan 9, 2020
@bradzacher
Copy link
Copy Markdown
Member

bradzacher commented Jan 9, 2020

There wasn't a bug before, just relaxed types; so switching this to a feat, it's an enhancement and strictification of the types we already provided.

@bradzacher bradzacher merged commit d3d70a3 into typescript-eslint:master Jan 9, 2020
@armano2 armano2 deleted the esTreeNodeToTSNodeMap branch January 9, 2020 22:31
This was referenced Feb 8, 2020
Closed
Closed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bradzacher bradzacher bradzacher approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin 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.

2 participants

@armano2 @bradzacher