Skip to content

Some properties named typeParameters instead of typeArguments #146

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
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
6.0.0

Comments

@dsherret
Copy link

dsherret commented Jan 26, 2019 •
edited by armano2
Loading

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
    );
}

typescript-eslint/packages/typescript-estree/src/convert.ts

Lines 2045 to 2050 in 45d8ad3

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 team members to take a look labels Jan 26, 2019
@armano2
Copy link
Collaborator

armano2 commented Jan 27, 2019 •
edited
Loading

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 team members to take a look labels Jan 27, 2019
@dsherret
Copy link
Author

dsherret commented Jan 27, 2019 •
edited
Loading

@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
Collaborator

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

uniqueiniquity pushed a commit to uniqueiniquity/typescript-eslint that referenced this issue Jan 28, 2019
@devdoomari3 @JamesHenry
Fix: plugin crashing on function declaration without member.id (types…
b0f89b4 
…cript-eslint#146)
This was referenced Feb 1, 2019
Merged
Closed
@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
Collaborator

armano2 commented May 31, 2022

note: investigate before 6.0.0 release

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Jul 26, 2022
Merged
3 tasks
@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.
Assignees
No one assigned
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
Milestone
6.0.0
Development

Successfully merging a pull request may close this issue.

fix: rename typeParameters to typeArguments where needed JoshuaKGoldberg/typescript-eslint
4 participants
@armano2 @dsherret @JoshuaKGoldberg @bradzacher