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

AST cleanup: type parameter in TSMappedType #6433

Closed
bradzacher opened this issue Feb 7, 2023 · 4 comments
Closed

AST cleanup: type parameter in TSMappedType #6433

bradzacher opened this issue Feb 7, 2023 · 4 comments
Assignees
Labels
accepting prs Go ahead, send a pull request that resolves this issue AST PRs and Issues about the AST structure breaking change This change will require a new major version to be released
Milestone

Comments

@bradzacher
Copy link
Member

bradzacher commented Feb 7, 2023

type MappedType = {
  [Key in Type]: Value;
};

Mapped types are a self-contained thing in TS; they can declare exactly one signature - the index signature defining the mapping. They can declare no other properties.
As such we emit a special AST node for them - TSMappedType

export interface TSMappedType extends BaseNode {
type: AST_NODE_TYPES.TSMappedType;
typeParameter: TSTypeParameter;
readonly?: boolean | '-' | '+';
optional?: boolean | '-' | '+';
typeAnnotation?: TypeNode;
nameType: TypeNode | null;
}

The problem with this AST is that we emit a TSTypeParameter for the indexer.

[ Key in Type ] : Value;
  ^^^^^^^^^^^ TSTypeParameter
  ^^^ .name = Identifier
         ^^^^ .constraint = TypeNode

We emit this because the underlying TS AST uses a ts.TypeParameter node for this.

This is a pretty weird AST considering the following things:

  1. TSTypeParameter supports things not syntactically[1] valid in a mapped type indexer (eg variance sigils (in, out) and default value T = Default).
  2. .constraint is marked as optional on TSTypeParameter, even though it is a syntactically[1] required.
  3. in all other usages of TSTypeParameter the existence of the .constraint property implies the extends keyword: Name extends Constraint. However just for the TSMappedType the .constraint instead implies the in keyword: Name in Constraint.
    Note that in this location it is syntactically[1] invalid to use [Key extends Value].
  4. it's possible to define a key remapping in a mapped type. This remapping node is defined as a sibling of the TSTypeParameter, but is part of the TSMappedType node:
    type MappedType = {
      [ Key in Type as Remapping ]: Value;
        ^^^^^^^^^^^ TSTypeParameter
                       ^^^^^^^^^ TSMappedType.nameType
    };

[1] Note that by syntactically valid I mean fatal parser error from TS - not just a semantic error!

It's pretty clear to me that our AST shape here is wrong and should be changed.

I propose that we switch to the following AST shape:

 export interface TSMappedType extends BaseNode {
   type: AST_NODE_TYPES.TSMappedType;
-  typeParameter: TSTypeParameter;
+  key: Identifier;
+  constraint: TypeNode;
   readonly?: boolean | '-' | '+';
   optional?: boolean | '-' | '+';
   typeAnnotation?: TypeNode;
   nameType: TypeNode | null;
 }

Migration Plan
With this sort of breaking AST change it can be difficult to roll out to the ecosystem without breaking the plugin ecosystem that may rely on the existing shape.
I propose the following migration plan:

  1. implement the above additions (key and constraint) to the AST.
  2. use JSDoc to mark typeParameter as @deprecated.
  3. (optional) add test-time logging to warn consumers that they're using a deprecated, to-be-removed property.
  4. broadcast this change (perhaps via a blog post for easy sharing?).
  5. breaking change remove typeParameter in a major release.
@bradzacher bradzacher added breaking change This change will require a new major version to be released AST PRs and Issues about the AST structure labels Feb 7, 2023
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Apr 8, 2023
@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0 milestone Apr 8, 2023
@JoshuaKGoldberg
Copy link
Member

@bradzacher I think we can land this in v6 similar to the other property renames. Thoughts?

@bradzacher
Copy link
Member Author

There are a lot of changes in v6 already and this is a large change.
I'm happy to add it to the list though!

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jul 10, 2023

We didn't make it in time. I'll move this to v8.

@JoshuaKGoldberg JoshuaKGoldberg modified the milestones: 6.0.0, 7.0.0 Jul 10, 2023
@JoshuaKGoldberg
Copy link
Member

#7065 was merged into the v8 branch. ✅

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2024
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 breaking change This change will require a new major version to be released
Projects
None yet
Development

No branches or pull requests

2 participants