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.

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

Some properties named typeParameters instead of typeArguments #146

Closed
dsherret opened this issue Jan 26, 2019 · 12 comments · Fixed by #5384
Closed

Some properties named typeParameters instead of typeArguments #146

dsherret opened this issue Jan 26, 2019 · 12 comments · Fixed by #5384
Labels
accepting prs Go ahead, send a pull request that resolves this issue AST PRs and Issues about the AST structure enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree question Questions! (i.e. not a bug / enhancment / documentation)
Milestone

Comments

@dsherret
Copy link

dsherret commented Jan 26, 2019

I've noticed several parts of the code referring to type arguments as type parameters. What's the reason for using typeParameters here? Seems like there might be some temporary stuff going on? Will this be renamed in the future?

if (node.typeArguments && node.typeArguments.length) {
    result.typeParameters = convertTypeArgumentsToTypeParameters(
        node.typeArguments
    );
}

if (node.typeArguments) {
result.typeParameters = this.convertTypeArgumentsToTypeParameters(
node.typeArguments,
node,
);
}

Some example code: playground

someFunction<string>(); // this type argument is incorrectly referred to as a type parameter in the ast
someFunction(1); // this argument is correctly referred to as an argument in the ast

Anyway, I just wanted to start a discussion about it so at least it's in the issue tracker. Also, FWIW, babel-eslint9 correctly refers to them as typeArguments when I'm looking at https://astexplorer.net, but I'm not so familiar with the babel or eslint ecosystem so I'm not sure how they're related (Edit: Looks like they call it node.typeArguments.params though...).

@dsherret dsherret added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for maintainers to take a look labels Jan 26, 2019
@armano2
Copy link
Member

armano2 commented Jan 27, 2019

this is good point, i did a lot of renaming for consistency in last few weeks, and i missed this one

@armano2 armano2 added enhancement New feature or request question Questions! (i.e. not a bug / enhancment / documentation) and removed triage Waiting for maintainers to take a look labels Jan 27, 2019
@dsherret
Copy link
Author

dsherret commented Jan 27, 2019

@armano2 sorry, I think my bad (first day looking at all this code). This looks like a question I should be raising in babel-parser? See here:

https://github.com/babel/babel/blob/03230eaa9ce198bdd9dcff048ab78f52d85ec55b/packages/babel-parser/src/plugins/typescript.js#L1532

Edit: Also in babel-types... https://github.com/babel/babel/blob/4c2f8d9337c2c45da345e536c1f44157d64a14bd/packages/babel-types/src/definitions/typescript.js#L83

@dsherret
Copy link
Author

I opened babel/babel#9418 ...I'm not sure how this project relates to babel's parser (haven't looked into it much).

@armano2
Copy link
Member

armano2 commented Jan 27, 2019

we are trying to make AST ~same as babel to improve user experience, but we have some changes to AST
(they have broken stuff), i'm regularly reporting issues there (and time to time fixing them)

you can find know issues with babel AST here: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/tests/ast-alignment/utils.ts#L84

@JoshuaKGoldberg
Copy link
Member

@armano2 @bradzacher @dsherret following #120, is this still an issue for anywhere in particular? I see a lot of functions like convertTypeArgumentsToTypeParameters in typescript-estree/src/convert.ts but haven't spotted any reason to keep this issue open?

@bradzacher
Copy link
Member

I have no idea tbh - we probably need to review the entire AST to figure out!

@armano2 armano2 added this to the 6.0.0 milestone May 31, 2022
@armano2
Copy link
Member

armano2 commented May 31, 2022

note: investigate before 6.0.0 release

@JoshuaKGoldberg
Copy link
Member

I threw up a draft of what this could look like on our end: #5384

Unless I've misinterpreted the code, it looks like we'll need Babel to also apply these suggested changes (babel/babel#9418) first. Marking as blocked until then.

@JoshuaKGoldberg JoshuaKGoldberg removed this from the 6.0.0 milestone Jul 26, 2022
@JoshuaKGoldberg JoshuaKGoldberg added the blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API label Jul 26, 2022
@bradzacher
Copy link
Member

We don't need babel to maintain compat with us. It's nice if they do but there's no requirement.

We already have a number of divergences in the AST which makes us incompatible.

@JoshuaKGoldberg
Copy link
Member

Oh, interesting - how should we change the failing spec tests in #5384 then?

@bradzacher
Copy link
Member

@JoshuaKGoldberg this file defines the set of "ignored" fixtures - we can just add the failing fixtures.

Ideally we should prioritise migrating all the tests across to the ast-spec package so that they can just be automatically listed in our spec differences:

@bradzacher bradzacher added question Questions! (i.e. not a bug / enhancment / documentation) AST PRs and Issues about the AST structure and removed question Questions! (i.e. not a bug / enhancment / documentation) labels Aug 13, 2022
@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labels Feb 5, 2023
@JoshuaKGoldberg
Copy link
Member

#5384 was merged into v6 and will be available as part of the v6 release. 🚀

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0 milestone Feb 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue AST PRs and Issues about the AST structure enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree question Questions! (i.e. not a bug / enhancment / documentation)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants