Skip to content

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

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

Closed
hi-ogawa opened this issue Feb 18, 2025 · 2 comments · Fixed by #10576
Closed
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug P-high Priority - High

Comments

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 18, 2025

reproduction: https://github.com/hi-ogawa/reproductions/tree/main/unocss-countable-set

I found a somewhat odd code on unocss while testing it with rolldown-vite https://github.com/unocss/unocss/blob/0a32090ddf452ef78ad5bedb6d5e888a161fefc9/packages-engine/core/src/utils/countable-set.ts. A simplified version is the following:

class CountableSet<K> extends Set<K> {
  _map: Map<K, number>

  constructor(values?: Iterable<K>) {
    super(values)  // `super` calls `add` which initialize `this._map`
    this._map ??= new Map()
  }

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

console.log(new CountableSet(["foo"]))
// expected to output
// CountableSet(1) [Set] { 'foo', _map: Map(1) { 'foo' => 1 } }

This only works when typescript useDefineForClassFields: false, which entirely strips class field parts typescript playground

class CountableSet extends Set {
    constructor(values) {
        super(values);
        this._map ?? (this._map = new Map());
    }
   ...

On rolldown-vite, I wasn't able to find a way to achieve this and it's likely because Oxc's setPublicClassFields: true resets _map after super calls.

class CountableSet extends Set {
        constructor(values) {
                super(values);
                this._map = void 0; 
                this._map ?? (this._map = new Map());
        }
...
console.log(new CountableSet(["foo"]))
// rolldown-vite output
// CountableSet(1) [Set] { 'foo', _map: Map(0) {} }
@hi-ogawa
Copy link
Contributor Author

Probably this isn't critical and the code shouldn't probably depend on such behavior even on useDefineForClassFields: false. For unocss case, it's actually broken on published version, so I suggested a fix there to eliminate such class field behavior dependence unocss/unocss#4436.

@Boshen Boshen added the P-high Priority - High label Apr 14, 2025
@sapphi-red sapphi-red added the A-transformer Area - Transformer / Transpiler label Apr 24, 2025
graphite-app bot pushed a commit that referenced this issue Apr 25, 2025
…lizer` option (#10576)

* close #9192
* close #10491

We've discussed adding `removeClassFieldsWithoutInitializer` option to support removing class fields without an initializer in #10491 (comment). This is used to align the`TypeScript`'s `useDefineForClassFields: false` option.
@Dunqing
Copy link
Member

Dunqing commented Apr 25, 2025

For TypeScript, if you wanted a behavior equivalent to useDefineForClassFields: false, you should set both set_public_class_fields and TypeScript's remove_class_fields_without_initializer to true.

/// When using public class fields, assume that they don't shadow any getter in the current class,
/// in its subclasses or in its superclass. Thus, it's safe to assign them rather than using
/// `Object.defineProperty`.
///
/// For example:
///
/// Input:
/// ```js
/// class Test {
/// field = 2;
///
/// static staticField = 3;
/// }
/// ```
///
/// When `set_public_class_fields` is `true`, the output will be:
/// ```js
/// class Test {
/// constructor() {
/// this.field = 2;
/// }
/// }
/// Test.staticField = 3;
/// ```
///
/// Otherwise, the output will be:
/// ```js
/// import _defineProperty from "@oxc-project/runtime/helpers/defineProperty";
/// class Test {
/// constructor() {
/// _defineProperty(this, "field", 2);
/// }
/// }
/// _defineProperty(Test, "staticField", 3);
/// ```
///
/// NOTE: For TypeScript, if you wanted behavior is equivalent to `useDefineForClassFields: false`, you should
/// set both `set_public_class_fields` and [`crate::TypeScriptOptions::remove_class_fields_without_initializer`]
/// to `true`.
#[serde(default)]
pub set_public_class_fields: bool,

@Dunqing Dunqing closed this as completed Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug P-high Priority - High
Projects
None yet
4 participants