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

useDefineForClassFields not respected in decorators for es2022 #2220

Closed
bennypowers opened this issue May 3, 2022 · 7 comments
Closed

useDefineForClassFields not respected in decorators for es2022 #2220

bennypowers opened this issue May 3, 2022 · 7 comments

Comments

@bennypowers
Copy link

bennypowers commented May 3, 2022

When useDefineForClassFields is set, typescript-decorated fields should not be output as ecmascript fields, as this will most likely break the resulting code. For example, lit decorators require useDefineForClassFields to be false, and rely on typescript compiling them to helper calls instead of ecmascript class fields

$ node --version
v16.13.0
$ npx esbuild --version
0.14.38
$ npx tsc --version
Version 4.6.4

tsconfig.json

{
  "compilerOptions": {
    "module": "es2022",
    "target": "es2022",
    "useDefineForClassFields": false,
    "experimentalDecorators": true,

    "declaration": true,
    "moduleResolution": "Node",
    "preserveValueImports": true,
    "strict": true,
}

Input:

import { LitElement, html } from 'lit';
import { customElement, query } from 'lit/decorators.js';

@customElement('pfe-avatar')
export class PfeAvatar extends LitElement {
  @query('canvas') private canvas?: HTMLCanvasElement;

  render() {
    return html`
      <canvas part="canvas"></canvas>
    `;
  }
}
Output:
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __decorateClass = (decorators, target, key, kind) => {
  var result = kind > 1 ? void 0 : kind ? __getOwnPropDesc(target, key) : target;
  for (var i = decorators.length - 1, decorator; i >= 0; i--)
    if (decorator = decorators[i])
      result = (kind ? decorator(target, key, result) : decorator(result)) || result;
  if (kind && result)
    __defProp(target, key, result);
  return result;
};
import { LitElement, html } from "./../../node_modules/lit/index.js";
import { customElement, query } from "./../../node_modules/lit/decorators.js";
export let PfeAvatar = class extends LitElement {
  canvas;
  render() {
    return html`
      <canvas part="canvas"></canvas>
    `;
  }
};
__decorateClass([
  query("canvas")
], PfeAvatar.prototype, "canvas", 2);
PfeAvatar = __decorateClass([
  customElement("pfe-avatar")
], PfeAvatar);
Expected:
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __decorateClass = (decorators, target, key, kind) => {
  var result = kind > 1 ? void 0 : kind ? __getOwnPropDesc(target, key) : target;
  for (var i = decorators.length - 1, decorator; i >= 0; i--)
    if (decorator = decorators[i])
      result = (kind ? decorator(target, key, result) : decorator(result)) || result;
  if (kind && result)
    __defProp(target, key, result);
  return result;
};
import { LitElement, html } from "./../../node_modules/lit/index.js";
import { customElement, query } from "./../../node_modules/lit/decorators.js";
export let PfeAvatar = class extends LitElement {
  render() {
    return html`
      <canvas part="canvas"></canvas>
    `;
  }
};
__decorateClass([
  query("canvas")
], PfeAvatar.prototype, "canvas", 2);
PfeAvatar = __decorateClass([
  customElement("pfe-avatar")
], PfeAvatar);

Diff:

--- a.js        2022-05-03 14:17:22.000000000 +0300
+++ b.js        2022-05-03 14:17:17.000000000 +0300
@@ -12,7 +12,6 @@
 import { LitElement, html } from "./../../node_modules/lit/index.js";
 import { customElement, query } from "./../../node_modules/lit/decorators.js";
 export let PfeAvatar = class extends LitElement {
-  canvas;
   render() {
     return html`
       <canvas part="canvas"></canvas>

Compare to tsc output:

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};
import { LitElement, html } from 'lit';
import { customElement, query } from 'lit/decorators.js';
let PfeAvatar = class PfeAvatar extends LitElement {
    render() {
        return html `
      <canvas part="canvas"></canvas>
    `;
    }
};
__decorate([
    query('canvas')
], PfeAvatar.prototype, "canvas", void 0);
PfeAvatar = __decorate([
    customElement('pfe-avatar')
], PfeAvatar);
export { PfeAvatar };

In other words, I expect esbuild to not add the canvas; javascript class field
This also occurs with "target": "es2020"

@bennypowers bennypowers changed the title useDefineForClassFields not respected for es2022 useDefineForClassFields not respected in decorators for es2022 May 3, 2022
bennypowers added a commit to patternfly/patternfly-elements that referenced this issue May 3, 2022
@bennypowers
Copy link
Author

workaround for @web/dev-server-esbuild users is to explicity set es2020 in the plugin config:

// web-dev-server.config.js
export default {
  plugins: [
    esbuildPlugin({ ts: true, target: 'es2020' }),
  ]
}

bennypowers added a commit to patternfly/patternfly-elements that referenced this issue May 3, 2022
bennypowers added a commit to patternfly/patternfly-elements that referenced this issue May 3, 2022
@hyrious
Copy link

hyrious commented May 3, 2022

I couldn't reproduce this in my local test, here's what I did:

> cat a.ts
import { LitElement, html } from 'lit';
import { customElement, query } from 'lit/decorators.js';

@customElement('pfe-avatar')
export class PfeAvatar extends LitElement {
  @query('canvas') private canvas?: HTMLCanvasElement;

  render() {
    return html`
      <canvas part="canvas"></canvas>
    `;
  }
}

> cat tsconfig.json
{
  "compilerOptions": {
    "module": "es2022",
    "target": "es2022",
    "useDefineForClassFields": false,
    "experimentalDecorators": true,

    "declaration": true,
    "moduleResolution": "Node",
    "preserveValueImports": true,
    "strict": true
  }
}

> esbuild --version
0.14.38

> esbuild a.ts
...
export let PfeAvatar = class extends LitElement {
  render() {
    return html`
      <canvas part="canvas"></canvas>
    `;
  }
};
...

As you can see, there's no canvas; in transform output.

@bennypowers
Copy link
Author

I also had trouble reproducing

I'll investigate whether this is a web-dev-server issue

@philkunz
Copy link

philkunz commented May 4, 2022

this is actually the same issue like #2219

@jarrodek
Copy link

jarrodek commented May 6, 2022

I have the same issue. After setting the 2020 target it is now working again,

@evanw
Copy link
Owner

evanw commented May 7, 2022

The only way I can reproduce this is to delete tsconfig.json and use esbuild --target=es2022 instead. That's analogous to using tsc --target es2022, which also emits the canvas field and has the same problem. So esbuild is behaving correctly here in that it's behaving the same way the TypeScript compiler behaves.

I'm guessing your problem is that somehow the tsconfig.json data isn't making it to esbuild through the wrapper? Or it's possible that there's an esbuild bug that's discarding the information somewhere. But to fix that you'd need to provide here the input to esbuild that produces the incorrect output without any other wrappers involved, which might take some digging on your end.

Regardless of where the problem is, a workaround is using declare before the field like this:

-  @query('canvas') private canvas?: HTMLCanvasElement;
+  @query('canvas') declare private canvas?: HTMLCanvasElement;

That tells TypeScript (and therefore esbuild) that the class field initializer should not be emitted.

@evanw
Copy link
Owner

evanw commented Jun 15, 2022

I'm closing this issue because I believe esbuild is behaving correctly here, this issue does not have a reproducible test case, and no follow-up reply was posted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants