denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
97.73k stars 5.38k forks source link

Class property decorators don't seem to work #14329

Closed dadooda closed 2 years ago

dadooda commented 2 years ago

Hey guys,

I've run into issues trying to use class property decorators. The following example demonstrates the issue (deco.ts):

function deco(): PropertyDecorator {
  return (target: Object, key: PropertyKey) => {
    Object.defineProperty(target, key, {
      get() { return "Joe" }
    })
  }
}

class Klass {
  @deco()
  name?: string
}

const obj = new Klass
console.log("obj.name", obj.name)
  1. When run in 🔨TS playground, the output is as expected: "obj.name", "Joe".
  2. When run as deno run deco.ts the output is: "obj.name", undefined.
  3. When run as deno bundle deco.ts 1.js && deno run 1.js by Deno v1.18.0, the output is:
    
    error: Uncaught TypeError: Cannot redefine property: name
            Object.defineProperty(target, property, desc1);
                ^
        at Function.defineProperty (<anonymous>)
        at _applyDecoratedDescriptor (file:///.../1.js:28:16)
        at file:///.../1.js:56:44    ```
  4. When run as deno bundle deco.ts 2.js && deno run 2.js by Deno v1.20.6, the output is: "obj.name", undefined.

Cheers! Alex

dadooda commented 2 years ago

Own amendment, randomly discovered after hours of trying.

Seems to work if the property is declared rather than defined:

@deco()
declare name?: string

I still think that the issue is there, despite the fact a workaround has been discovered.

aapoalas commented 2 years ago

I believe this is from a common surprise that TypeScript implements:

class Foo {
  prop?: string;
}

transpiles into

class Foo {
  constructor() {
    this.prop = undefined;
  }
}

or something along those lines.

EDIT: This class purely does not transpile into a class with constructor, but I ran into something like this elsewhere with TS transpilation where declare was then required. Maybe it had to do with extending and whatnot?

willmartian commented 2 years ago

A comment on the above issue by @tdillon that might be helpful, where he notes that the issue was briefly resolved in v1.17.2:

Deno 1.17.2 has resolved my issue. Besides some very minor formatting differences, the output of deno bundle in version 1.17.2 includes the following code which does not exist in the output from 1.17.1.

var own = Object.getOwnPropertyDescriptor(target, property);
if (own && (own.get || own.set)) {
    delete desc1.writable;
    delete desc1.initializer;
willmartian commented 2 years ago

Nice, it looks like this issue might have been resolved in swc v1.2.175: https://github.com/swc-project/swc/issues/2655

I am not familiar with Rust, so I am unsure how to check what version Deno is using. Could someone verify the version?

aadamsx commented 2 years ago

@willmartian I'm looking to add a Lit component for SSR on Deno. Will this issue cause me issues? Will we need to wait until swc v1.2.175 is incorporated into Deno for Lit to work with Deno?

kitsonk commented 2 years ago

See: #13281

TypeScript decorators are incompatible with class fields. We force the use of class fields in Deno, because that is modern and proper JavaScript and matches universal runtime behaviour between JavaScript and TypeScript. The experimental decorators in TypeScript were built upon old TypeScript class fields, which have different runtime behaviour and semantics, and neither swc nor the TypeScript emit can get them to work together.

The use of declare in TypeScript means that it is not properly a class field, and therefore something can be emitted that works, because there is no class field to "obscure" the application of the experimental decorator.

Any sort of warning that experimental TypeScript decorators are incompatible with class fields would be something that should be taken up with TypeScript, and if accepted, Deno would update, and then issue diagnostics that they are incompatible.

willmartian commented 2 years ago

Thanks for the clarification @kitsonk!