Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Issue with property named get inside classes #296

Closed
yspreen opened this issue Mar 25, 2024 · 9 comments
Closed

Issue with property named get inside classes #296

yspreen opened this issue Mar 25, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@yspreen
Copy link

yspreen commented Mar 25, 2024

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);
@richarddavison
Copy link
Collaborator

richarddavison commented Mar 25, 2024

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())

@yspreen
Copy link
Author

yspreen commented Mar 26, 2024

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

new TestClass().get();

@yspreen
Copy link
Author

yspreen commented Mar 26, 2024

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

@richarddavison richarddavison added the bug Something isn't working label Mar 26, 2024
@richarddavison
Copy link
Collaborator

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:
bellard/quickjs#258

@nabetti1720
Copy link
Contributor

nabetti1720 commented Aug 10, 2024

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", ...

@richarddavison
Copy link
Collaborator

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

@nabetti1720
Copy link
Contributor

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

@richarddd
Copy link

Should work in latest release due to updated QJS version. Can you check again @yspreen ?

@richarddavison
Copy link
Collaborator

This is now resolved!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants