fix(transformer/class-properties): align setPublicClassFields with TypeScript's useDefineForClassFields: false

[Dunqing]

• close assumptions.setPublicClassFields doesn't fully simulate tsconfig useDefineForClassFields #9192

Do not need to convert the property without an initializer when setPublicClassFields is true.

Input:

class Cls {
  // `prop1` without initialzier
  prop1: number;
  // `prop2` initializes as "str"
  prop2: string = "str"
}

Before output:

class Cls {
    constructor() {
        // `prop1` without initialzier
        this.prop1 = void 0;
        // `prop2` initializes as "str"
        this.prop2 = "str";
    }
}

After output:

class Cls {
    constructor() {
        // `prop2` initializes as "str"
        this.prop2 = "str";
    }
}

The behaviour is the same as TypeScript now.

I am not sure whether we should align this behavior with TypeScript because this will break cases like 'prop1' in (new Cls()). But if not, we will break #9192 the issue, though it is not used correctly. Considering setPublicClassFields is equivalent to useDefineForClassFields: false and people always regard both are the same, so personally I think align is correct.

[Dunqing]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(transformer/class-properties): align setPublicClassFields with TypeScript's useDefineForClassFields: false #10491 👈 (View in Graphite)
• test(transformer/class-properties): overrides a few tests that contain class properties without initializer usages #10538
• refactor(transformer/class-properties): simplify to get constructor method only once #10493
• refactor(transformer/class-properties): defer re-parenting initializers scopes until all instance properties are transformed #10495
• refactor(transformer/class-properties): streamline handling scope of instance property initializer #10492
• main

---

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.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #10491 will create unknown performance changes

_{Comparing 04-18-fix_transformer_class-properties_align_setpublicclassfields_with_typescript_s_usedefineforclassfields (b9f2d63) with main (7e71282)}

Summary

🆕 36 new benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
🆕	codegen[checker.ts]	N/A	22.2 ms	N/A
🆕	codegen_sourcemap[checker.ts]	N/A	65.1 ms	N/A
🆕	formatter[antd.js]	N/A	708.3 ms	N/A
🆕	formatter[react.development.js]	N/A	8 ms	N/A
🆕	formatter[typescript.js]	N/A	1.1 s	N/A
🆕	isolated-declarations[vue-id.ts]	N/A	58.4 ms	N/A
🆕	lexer[RadixUIAdoptionSection.jsx]	N/A	21.3 µs	N/A
🆕	lexer[antd.js]	N/A	24.8 ms	N/A
🆕	lexer[cal.com.tsx]	N/A	5.9 ms	N/A
🆕	lexer[checker.ts]	N/A	14.9 ms	N/A
🆕	lexer[pdf.mjs]	N/A	3.9 ms	N/A
🆕	linter[RadixUIAdoptionSection.jsx]	N/A	2.7 ms	N/A
🆕	linter[cal.com.tsx]	N/A	1.2 s	N/A
🆕	linter[checker.ts]	N/A	3 s	N/A
🆕	mangler[antd.js]	N/A	15.9 ms	N/A
🆕	mangler[react.development.js]	N/A	293.9 µs	N/A
🆕	mangler[typescript.js]	N/A	39.4 ms	N/A
🆕	minifier[antd.js]	N/A	163.6 ms	N/A
🆕	minifier[react.development.js]	N/A	1.8 ms	N/A
🆕	minifier[typescript.js]	N/A	287.9 ms	N/A
...	...	...	...	...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

[overlookmotel]

This is tricky. setPublicClassFields and useDefineForClassFields aren't actually the same thing.

Here's the example from #9192 as vanilla JS:

class CountableSet extends Set {
  _map;

  constructor(values) {
    super(values);
  }

  add(key) {
    this._map ??= new Map();
    this._map.set(key, (this._map.get(key) ?? 0) + 1);
    return super.add(key);
  }
}

const c = new CountableSet(['x']);
assert(c._map === undefined);

Run in NodeJS, the assertion passes. And, before this PR, in the Oxc-transformed version it passes (regardless of whether setPublicClassFields option is set or not). But after this PR, the assertion fails, meaning it's an incorrect transform. Presumably Babel had test cases for this to ensure the transform doesn't alter behavior.

So this PR fixes TypeScript, but breaks JS!

Should we maybe:

1. Add a separate useDefineForClassFields option which follows the TS behavior? or
2. Make setPublicClassFields align with useDefineForClassFields only if source type is TS?

[overlookmotel]

The specific issue which motivated #9192 is now fixed upstream: unocss/unocss#4436. Does that make this a bit less urgent, so we have time to discuss what we want to do?

Or also I don't object to merging this now, and fixing JS transform in a follow-on.

[Dunqing]

This is tricky. setPublicClassFields and useDefineForClassFields aren't actually the same thing.

Here's the example from #9192 as vanilla JS:

class CountableSet extends Set {
  _map;

  constructor(values) {
    super(values);
  }

  add(key) {
    this._map ??= new Map();
    this._map.set(key, (this._map.get(key) ?? 0) + 1);
    return super.add(key);
  }
}

const c = new CountableSet(['x']);
assert(c._map === undefined);

Your example missed this._map ??= new Map() line after super(values);.

The correct example is:

class CountableSet extends Set {
  _map;

  constructor(values) {
    super(values);
    this._map ??= new Map();
  }

  add(key) {
    this._map ??= new Map();
    this._map.set(key, (this._map.get(key) ?? 0) + 1);
    return super.add(key);
  }
}

const c = new CountableSet(['x']);
assert(c._map === undefined);

Also, you shouldn't run code by node without transforming because this code relies on useDefineForClassFields: false. And, useDefineForClassFields: true is the standard behavior, and node follows it. If you want to run, use the code TypeScript transformed.

Should we maybe:

1. Add a separate useDefineForClassFields option which follows the TS behavior? or
2. Make setPublicClassFields align with useDefineForClassFields only if source type is TS?

I prefer 2, because this is exactly what the .ts files want.

@hi-ogawa @sapphi-red @sxzz, any thoughts here. It seems that all of you are paying attention to this useDefineForClassFields

[sapphi-red]

It seems allowDeclareFields changes the behavior in babel here 🫠

TypeScript (useDefineForClassFields=false)

class CountableSet extends Set {
    constructor(values) {
        super(values);
        this._map ?? (this._map = new Map());
    }
    add(key) {
        this._map ?? (this._map = new Map());
        this._map.set(key, (this._map.get(key) ?? 0) + 1);
        return super.add(key);
    }
}
const c = new CountableSet(['x']);
assert(c._map === undefined);

esbuild outputs the same.
babel with setPublicClassFields: false + allowDeclareFields: false outputs the same.
babel with setPublicClassFields: true + allowDeclareFields: false outputs the same.

TypeScript (useDefineForClassFields=true)

class CountableSet extends Set {
    constructor(values) {
        super(values);
        Object.defineProperty(this, "_map", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: void 0
        });
        this._map ?? (this._map = new Map());
    }
    add(key) {
        this._map ?? (this._map = new Map());
        this._map.set(key, (this._map.get(key) ?? 0) + 1);
        return super.add(key);
    }
}
const c = new CountableSet(['x']);
assert(c._map === undefined);

esbuild outputs the same.
babel with setPublicClassFields: false + allowDeclareFields: true outputs the same.

babel with setPublicClassFields: true + allowDeclareFields: true

class CountableSet extends Set {
  constructor(values) {
    super(values);
    this._map = void 0;
    this._map ??= new Map();
  }
  add(key) {
    this._map ??= new Map();
    this._map.set(key, (this._map.get(key) ?? 0) + 1);
    return super.add(key);
  }
}
const c = new CountableSet(['x']);
assert(c._map === undefined);

stackblitz

[sapphi-red]

For this input

class CountableSet extends Set {
  _map = null;

  constructor(values) {
    super(values);
    this._map ??= new Map();
  }

  add(key) {
    this._map ??= new Map();
    this._map.set(key, (this._map.get(key) ?? 0) + 1);
    return super.add(key);
  }
}

const c = new CountableSet(['x']);
assert(c._map === undefined);

The output for setPublicClassFields: false + allowDeclareFields: false is:

function _defineProperty(e, r, t) { return (r = _toPropertyKey(r)) in e ? Object.defineProperty(e, r, { value: t, enumerable: !0, configurable: !0, writable: !0 }) : e[r] = t, e; }
function _toPropertyKey(t) { var i = _toPrimitive(t, "string"); return "symbol" == typeof i ? i : i + ""; }
function _toPrimitive(t, r) { if ("object" != typeof t || !t) return t; var e = t[Symbol.toPrimitive]; if (void 0 !== e) { var i = e.call(t, r || "default"); if ("object" != typeof i) return i; throw new TypeError("@@toPrimitive must return a primitive value."); } return ("string" === r ? String : Number)(t); }
class CountableSet extends Set {
  constructor(values) {
    super(values);
    _defineProperty(this, "_map", null);
    this._map ??= new Map();
  }
  add(key) {
    this._map ??= new Map();
    this._map.set(key, (this._map.get(key) ?? 0) + 1);
    return super.add(key);
  }
}
const c = new CountableSet(['x']);
assert(c._map === undefined);

and the output for setPublicClassFields: true + allowDeclareFields: false is:

class CountableSet extends Set {
  constructor(values) {
    super(values);
    this._map = null;
    this._map ??= new Map();
  }
  add(key) {
    this._map ??= new Map();
    this._map.set(key, (this._map.get(key) ?? 0) + 1);
    return super.add(key);
  }
}
const c = new CountableSet(['x']);
assert(c._map === undefined);

---

I guess setPublicClassFields: true + allowDeclareFields: false is equivalent to useDefineForClassFields=false, and setPublicClassFields: false + allowDeclareFields: true is equivalent to useDefineForClassFields=true 🤔

[Dunqing]

I guess setPublicClassFields: true + allowDeclareFields: false is equivalent to useDefineForClassFields=false, and setPublicClassFields: false + allowDeclareFields: true is equivalent to useDefineForClassFields=true 🤔

Thank you for taking the time to review this! Yes, you are right! It's the same as your thought, see Babel implementation, but I haven't seen any documentation mention this.

It's pretty weird that Babel completes alignment with useDefineForClassFields by setPublicClassFields: false and allowDeclareFields: false options among the class-properties and typescript plugins. And I don't think removing properties without an initializer is allowDeclareFields responsible, also does not need an option to allow using declare modifier.

But anyway, we need to consider how to align it. How do we provide the useDefineForClassFields option in the TypeScript plugin? This will set both setPublicClassFields and allowDeclareFields to false. Also, currently, Oxc does nothing for the allowDeclareFields option yet, and Babel's implementation has a little problem, they won't remove property that has decorators.

[sapphi-red]

How do we provide the useDefineForClassFields option in the TypeScript plugin? This will set both setPublicClassFields and allowDeclareFields to false. Also, currently, Oxc does nothing for the allowDeclareFields option yet, and Babel's implementation has a little problem, they won't remove property that has decorators.

I think Oxc can have two options to control the behavior:

• setPublicClassFields: same with babel's behavior
• removePublicClassFieldsWithoutInitializer: When true, remove public class fields without initializer. Default to false. The opposite of allowDeclareFields, but the declare keyword is allowed in TypeScript files regardless of this option.

Then, document that if a user wants to align with

• useDefineForClassFields=false, they should set setPublicClassFields=true + removePublicClassFieldsWithoutInitializer=true
• useDefineForClassFields=true, they should set setPublicClassFields=false + removePublicClassFieldsWithoutInitializer=false (default)

For Vite's usecase, the minimal requirements is that there's a way to behave like useDefineForClassFields=false and useDefineForClassFields=true though.

[Dunqing]

I think Oxc can have two options to control the behavior:

• setPublicClassFields: same with babel's behavior
• removePublicClassFieldsWithoutInitializer: When true, remove public class fields without initializer. Default to false. The opposite of allowDeclareFields, but the declare keyword is allowed in TypeScript files regardless of this option.

Then, document that if a user wants to align with

• useDefineForClassFields=false, they should set setPublicClassFields=true + removePublicClassFieldsWithoutInitializer=true
• useDefineForClassFields=true, they should set setPublicClassFields=false + removePublicClassFieldsWithoutInitializer=false (default)

For Vite's usecase, the minimal requirements is that there's a way to behave like useDefineForClassFields=false and useDefineForClassFields=true though.

Sounds good! Also, I'm going to deprecate allowDeclareFields, no reason we need this option once we have removePublicClassFieldsWithoutInitializer, and I don't think declare properties need to be supported by an option. I guess they had added this option for "removing properties without an initializer". Does this make sense to you?

[sapphi-red]

Makes sense to me 👍