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

fix(core): methodsOf filters out getters and symbols #2983

Merged
merged 1 commit into from
Feb 6, 2025
Merged

fix(core): methodsOf filters out getters and symbols #2983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions packages/core/src/utils/objects/methodsOf.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import {methodsOf} from "./methodsOf.js";

describe("methodsOf()", () => {
it("should return methods", () => {
class TestClass {
method() {}

property: string;
}

const methods = methodsOf(TestClass);
expect(methods).toHaveLength(1);
expect(methods[0]).toMatchObject({
propertyKey: "method"
});
});
it("should not return getters", () => {
class TestClass {
method() {}

get property() {
return "";
}
}

const methods = methodsOf(TestClass);

expect(methods).toHaveLength(1);
expect(methods[0]).toMatchObject({
propertyKey: "method"
});
});
Comment on lines +17 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding more test cases for comprehensive coverage.

While the getter test is good, consider adding tests for:

  • Symbol methods
  • Constructor method
  • Multiple methods in a class
  • Inherited methods from parent classes

Here's a suggested test case for symbols and inheritance:

it("should handle symbols and inheritance", () => {
  const symbolMethod = Symbol("symbolMethod");
  
  class ParentClass {
    parentMethod() {}
  }
  
  class TestClass extends ParentClass {
    [symbolMethod]() {}
    method() {}
  }

  const methods = methodsOf(TestClass);
  
  expect(methods).toHaveLength(2);
  expect(methods.map(m => m.propertyKey)).toEqual(["method", "parentMethod"]);
});

});
7 changes: 4 additions & 3 deletions packages/core/src/utils/objects/methodsOf.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {Type} from "../../domain/Type.js";
import {ancestorsOf} from "./ancestorsOf.js";
import {classOf} from "./classOf.js";
import {isSymbol} from "./isSymbol.js";
import {prototypeOf} from "./prototypeOf.js";

/**
Expand All @@ -15,9 +16,9 @@ export function methodsOf(target: any): {target: Type; propertyKey: string}[] {
const keys = Reflect.ownKeys(prototypeOf(target));

keys.forEach((propertyKey: string) => {
if (propertyKey !== "constructor") {
methods.set(propertyKey, {target, propertyKey});
}
if (isSymbol(propertyKey) || propertyKey === "constructor" || Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey)?.get)
return;
methods.set(propertyKey, {target, propertyKey});
Comment on lines +19 to +21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider improving readability of the filtering condition.

While the implementation is correct, the condition could be more readable by splitting it into separate checks with descriptive variable names.

Consider this refactoring for improved readability:

-      if (isSymbol(propertyKey) || propertyKey === "constructor" || Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey)?.get)
-        return;
-      methods.set(propertyKey, {target, propertyKey});
+      const descriptor = Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey);
+      const isGetter = descriptor?.get !== undefined;
+      const isConstructor = propertyKey === "constructor";
+      
+      if (isSymbol(propertyKey) || isConstructor || isGetter) {
+        return; // Skip symbols, constructor, and getter properties
+      }
+      
+      methods.set(propertyKey, {target, propertyKey});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (isSymbol(propertyKey) || propertyKey === "constructor" || Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey)?.get)
return;
methods.set(propertyKey, {target, propertyKey});
const descriptor = Object.getOwnPropertyDescriptor(prototypeOf(target), propertyKey);
const isGetter = descriptor?.get !== undefined;
const isConstructor = propertyKey === "constructor";
if (isSymbol(propertyKey) || isConstructor || isGetter) {
return; // Skip symbols, constructor, and getter properties
}
methods.set(propertyKey, {target, propertyKey});

});
});

Expand Down
Loading