How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

CodSpeed Instrumentation Performance Report

Merging #10092 will not alter performance

_{Comparing fix/issue-10088 (fa97b6c) with main (e000f60)}

Summary

✅ 36 untouched benchmarks

Thanks for this.

Since we know for sure that the template string must have no expressions and only 1 quasi in this case, I wonder if we should add a specific type for it?

pub enum TSEnumMemberName<'a> {
    Identifier(Box<'a, IdentifierName<'a>>) = 0,
    String(Box<'a, StringLiteral<'a>>) = 1,
    Template(Box<'a, SimpleTemplateLiteral<'a>>) = 2,
}

/// A template literal with no expressions and only 1 quasi.
/// Basically a copy of `StringLiteral`, but with backtick quotes.
pub struct SimpleTemplateLiteral<'a> {
    span: Span,
    #[estree(via = StringLiteralValue)]
    pub value: Atom<'a>,
    #[content_eq(skip)]
    pub raw: Option<Atom<'a>>,
    #[builder(default)]
    #[estree(skip)]
    pub lone_surrogates: bool,
}

Personally I think that'd be preferable as, generally speaking, I think it's ideal for the AST to statically be unable to represent syntax which is illegal.

But @Boshen what do you think?

Additionally, instead of a computed property, we could choose to represent whether the property is computed or not with extra enum variants:

pub enum TSEnumMemberName<'a> {
    Identifier(Box<'a, IdentifierName<'a>>) = 0,
    String(Box<'a, StringLiteral<'a>>) = 1,
    Template(Box<'a, SimpleTemplateLiteral<'a>>) = 2,
    ComputedString(Box<'a, StringLiteral<'a>>) = 3,
    ComputedTemplate(Box<'a, SimpleTemplateLiteral<'a>>) = 4,
}

Again, the rationale is that this makes it impossible to represent an illegal syntax in AST:

enum Foo {
    // Illegal syntax - identifiers cannot be computed
    [x] = 1,
}

(there is no ComputedIdentifier variant)

This also makes TSEnumMember 8 bytes smaller than adding a computed: bool property.

Thanks for the review~!

I too prefer that idea because it is explicit.
But for computed property, it seems to be present in other AST nodes already, so which is better...?

Let's wait for the Boshen's decision. 🙏🏻

One other thing: Doesn't codegen need to wrap in [...] when computed: true to preserve the original syntax?

Otherwise this:

export enum X {
  [`A`] = 123,
}

is printed as:

export enum X {
  `A` = 123,
}

...which is invalid syntax.

@Boshen Could you please give your opinion on #10092 (comment)? Personally, I like the tighter constraint that a SimpleTemplateLiteral would give. But I know you're not keen to add more AST types.

I think adding a computed field is good, which is also compatible with typescript-eslint.

Rebased on main to fix merge conflicts. I'm going to merge this when CI passes.

I realized my idea of a SimpleTemplateLiteral is not good. The problem is that visitors for TemplateLiteral need to visit this node, and they won't if it's a different type.

I do think my other suggestion of removing the computed field and replacing with additional enum variants on TSEnumMemberName is good though:

pub enum TSEnumMemberName<'a> {
    Identifier(Box<'a, IdentifierName<'a>>) = 0,
    String(Box<'a, StringLiteral<'a>>) = 1,
    Template(Box<'a, SimpleTemplateLiteral<'a>>) = 2,
    ComputedString(Box<'a, StringLiteral<'a>>) = 3,
    ComputedTemplate(Box<'a, SimpleTemplateLiteral<'a>>) = 4,
}

I'll open an issue for making that change. But let's get this merged in meantime!