llrt icon indicating copy to clipboard operation
llrt copied to clipboard

Issue with property named `get` inside classes

Open yspreen opened this issue 1 year ago • 7 comments

Express and Drizzle both have classes with property methods called get. It seems to be fine if defined like

get(/* args */) {
  /* method */
}

but this line in drizzle: https://github.com/drizzle-team/drizzle-orm/blob/main/drizzle-orm/src/sqlite-core/query-builders/delete.ts#L251

get: ReturnType<this['prepare']>['get'] = (placeholderValues) => {
	return this._prepare().get(placeholderValues);
};

causes issues in LLRT when compiled with TS and bundled with rollup.js:

invalid property name

because the compiled result is

get = (e) => this._prepare().get(e);

yspreen avatar Mar 25 '24 02:03 yspreen

Hi @yspreen, thanks for the report. Do you have a minimum reproducible js file?

I can't reproduce this:

class Examle{
  get(){
      return "hello world")
  }
}

console.log(new Example().get())

richarddavison avatar Mar 25 '24 10:03 richarddavison

~/Downloads/llrt ./test.js
SyntaxError: invalid property name
    at ./test.js:2:6
class TestClass {
  get = () => console.log("get");
}

new TestClass().get();

yspreen avatar Mar 26 '24 02:03 yspreen

as mentioned above, it only breaks if you set it to an anonymous method. not if defining get() {}

yspreen avatar Mar 26 '24 02:03 yspreen

I've located the issue and submitted a patch to downstream QuickJS engine. The catch is that we're not up to date on the latest version. However, we can (and already do) apply patches independently of the engine: https://github.com/bellard/quickjs/pull/258

richarddavison avatar Apr 02 '24 09:04 richarddavison

Hi @richarddavison , I thought it was fundamentally the same problem, so I am adding it here.

The following code is part of the code generated when ES2022 is specified for a bundler in the Javascript framework called hono. The code is kept to a minimum so that it can be reproduced.

// reproduction.js
var Hono = class {
  get;
  post;
  put;
  delete;
  options;
  patch;
  //...
}
% llrt reproduction.js
SyntaxError: invalid property name
  at reproduction.js:2:5

Specifying ES2020 generates the following code, which can also be executed by LLRT.

var __defProp = Object.defineProperty;
var __defNormalProp = (obj, key, value) => key in obj ? __defProp(obj, key, { enumerable: true, configurable: true, writable: true, value }) : obj[key] = value;
var __publicField = (obj, key, value) => __defNormalProp(obj, typeof key !== "symbol" ? key + "" : key, value);
//...
var _path2, _a3;
var Hono = (_a3 = class {
  constructor(options = {}) {
    __publicField(this, "get");
    __publicField(this, "post");
    __publicField(this, "put");
    __publicField(this, "delete");
    __publicField(this, "options");
    __publicField(this, "patch");
//...

EDIT: rquickjs is the latest from git.

rquickjs = { version = "0.6.2", git = "https://github.com/DelSkayn/rquickjs", ...

nabetti1720 avatar Aug 10 '24 12:08 nabetti1720

There is also this that needs to be merged upstream: https://github.com/quickjs-ng/quickjs/pull/476 QuickJS and QuickJS-NG will converge in Q4 this year (planned)

richarddavison avatar Aug 30 '24 12:08 richarddavison

Thanks for the information. I had completely overlooked the quickjs-ng initiative. As for the newly cut out issue, I will close it. :)

nabetti1720 avatar Aug 30 '24 12:08 nabetti1720