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

Bug: NewExpression doesn't allow SpreadElement as argument #5421

Closed
4 tasks done
burtek opened this issue Aug 4, 2022 · 3 comments · Fixed by #5422
Closed
4 tasks done

Bug: NewExpression doesn't allow SpreadElement as argument #5421

burtek opened this issue Aug 4, 2022 · 3 comments · Fixed by #5422
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working

Comments

@burtek
Copy link
Contributor

burtek commented Aug 4, 2022

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

ast-spec

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

NewExpression and CallExpression are basically identical in terms of arguments list. Among others, you're perfectly fine with spreading arguments in both of them:

class A { constructor(...args: unknown[]) {} }
const a = new A(...someArray);

const F = (...args: unknown[]) => {}
const f = F(...someArray);

That is reflected in CallExpression type:

export interface CallExpression extends BaseNode {
type: AST_NODE_TYPES.CallExpression;
callee: LeftHandSideExpression;
arguments: CallExpressionArgument[];

With CallExpressionArgument being:
export type CallExpressionArgument = Expression | SpreadElement;

At the same time, NewExpression typing allows only Expression arguments:

export interface NewExpression extends BaseNode {
type: AST_NODE_TYPES.NewExpression;
callee: LeftHandSideExpression;
arguments: Expression[];

SpreadElement is not there, even though it is legal to spread arguments in constructor call (playground)

Fail

n/a

Pass

n/a

Additional Info

No response

@burtek burtek added enhancement New feature or request triage Waiting for maintainers to take a look labels Aug 4, 2022
@burtek
Copy link
Contributor Author

burtek commented Aug 4, 2022

Ofc happy to preapre a PR for this, but as this is my first contribution to typescript-eslint, I wanted to make sure this is discussed and that my thinking is correct

@bradzacher bradzacher added bug Something isn't working accepting prs Go ahead, send a pull request that resolves this issue and removed enhancement New feature or request triage Waiting for maintainers to take a look labels Aug 4, 2022
@bradzacher
Copy link
Member

This is definitely a bug in the types NewExpression should accept the same argument type as CallExpression!

@burtek
Copy link
Contributor Author

burtek commented Aug 4, 2022

Working on it :)

@burtek burtek changed the title Enhancement: Allow NewExpression arguments to be a SpreadElement. Bug: NewExpression doesn't allow SpreadElement as argument Aug 4, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2022
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 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants