From 1831e293960c2b6132d33099d227b94169e1c8d7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=EA=B9=80=EC=84=A0=EB=AF=BC?= <tjsals116@gmail.com>
Date: Fri, 29 Mar 2024 00:23:02 +0900
Subject: [PATCH] Fix multiple issues based on PR review feedback

- Comment: Seems like chainable would be enough here, since you can't chain empty several times
- Comment: I think we could make this more efficient by using a for in loop instead of Object.keys and breaking the loop by returning false if an object own property is encountered.
- Comment: It should just be Chainable here as well
- Comment: I'm not sure a new pattern type is necessary here because both patterns you added are implemented with guards
- Comment: Could you add test covering how P.object behaves with more inputs: Functions, Primitive values, Null. It should catch all values that are assignable to the object type, and type narrowing and exhaustive should both work
- Comment: Could you remove this diff?
---
 src/patterns.ts      |  9 +++++--
 src/types/Pattern.ts |  6 ++---
 tests/object.test.ts | 63 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/src/patterns.ts b/src/patterns.ts
index 4ef9eb7d..ebca0ea0 100644
--- a/src/patterns.ts
+++ b/src/patterns.ts
@@ -1128,14 +1128,19 @@ export function shape(pattern: UnknownPattern) {
  *   .with(P.object.empty(), () => 'will match on empty objects')
  */
 const emptyObject = <input>(): GuardExcludeP<input, object, never> => when(
-    (value) => value && typeof value === 'object' && Object.keys(value).length === 0,
+    (value) => {
+      if (!isObject(value)) return false;
+
+      for (var prop in value) return false;
+      return true;
+    },
   );
 
 const objectChainable = <pattern extends Matcher<any, any, any, any, any>>(
   pattern: pattern
 ): ObjectChainable<pattern> =>
   Object.assign(chainable(pattern), {
-    empty: () => objectChainable(intersection(pattern, emptyObject())),
+    empty: () => chainable(intersection(pattern, emptyObject())),
   }) as any;
 
 /**
diff --git a/src/types/Pattern.ts b/src/types/Pattern.ts
index d6fb8913..9809b605 100644
--- a/src/types/Pattern.ts
+++ b/src/types/Pattern.ts
@@ -15,7 +15,6 @@ export type MatcherType =
   | 'or'
   | 'and'
   | 'array'
-  | 'object'
   | 'map'
   | 'set'
   | 'select'
@@ -98,7 +97,7 @@ export type CustomP<input, pattern, narrowedOrFn> = Matcher<
 
 export type ArrayP<input, p> = Matcher<input, p, 'array'>;
 
-export type ObjectP<input, p> = Matcher<input, p, 'object'>;
+export type ObjectP<input, p> = Matcher<input, p>;
 
 export type OptionalP<input, p> = Matcher<input, p, 'optional'>;
 
@@ -194,6 +193,7 @@ export type NullishPattern = Chainable<
   GuardP<unknown, null | undefined>,
   never
 >;
+
 type MergeGuards<input, guard1, guard2> = [guard1, guard2] extends [
   GuardExcludeP<any, infer narrowed1, infer excluded1>,
   GuardExcludeP<any, infer narrowed2, infer excluded2>
@@ -676,7 +676,7 @@ export type ObjectChainable<
        * match(value)
        *  .with(P.object.empty(), () => 'empty object')
        */
-      empty<input>(): ObjectChainable<
+      empty<input>(): Chainable<
         ObjectP<input, Record<string, never>>,
         omitted | 'empty'
       >;
diff --git a/tests/object.test.ts b/tests/object.test.ts
index a2a45b75..93987a63 100644
--- a/tests/object.test.ts
+++ b/tests/object.test.ts
@@ -20,6 +20,66 @@ describe('Object', () => {
     expect(res).toEqual('not found');
   });
 
+  it('when input is a Function, it should not match as an exact object', () => {
+    const fn = () => () => {};
+
+    const res = match(fn())
+      .with(P.object, (obj) => {
+        type t = Expect<Equal<typeof obj, () => void>>;
+        return 'not found';
+      })
+      .otherwise(() => 'not found');
+    expect(res).toEqual('not found');
+  })
+
+  it('when input is a Number (a primitive value), it should not be matched as an exact object', () => {
+    const fn = () => 1_000_000;
+
+    const res = match(fn())
+      .with(P.object, (obj) => {
+        type t = Expect<Equal<typeof obj, number>>;
+        return 'not found';
+      })
+      .otherwise(() => 'not found');
+    expect(res).toEqual('not found');
+  })
+
+  it('when input is a String (a primitive value), it should not be matched as an exact object', () => {
+    const fn = () => 'hello';
+
+    const res = match(fn())
+      .with(P.object, (obj) => {
+        type t = Expect<Equal<typeof obj, string>>;
+        return 'not found';
+      })
+      .otherwise(() => 'not found');
+    expect(res).toEqual('not found');
+  })
+
+  it('when input is a Boolean (a primitive value), it should not be matched as an exact object', () => {
+    const fn = () => true;
+
+    const res = match(fn())
+      .with(P.object, (obj) => {
+        type t = Expect<Equal<typeof obj, boolean>>;
+        return 'not found';
+      })
+      .otherwise(() => 'not found');
+    expect(res).toEqual('not found');
+  })
+
+  it('when input is Null, it should not be matched as an exact object', () => {
+    const fn = () => null;
+
+    const res = match(fn())
+      .with(P.object, (obj) => {
+        type t = Expect<Equal<typeof obj, null>>;
+        return 'not found';
+      })
+      .otherwise(() => 'not found');
+    expect(res).toEqual('not found');
+  })
+
   it('should match object with nested objects', () => {
     const res = match({ x: { y: 1 } })
       .with({ x: { y: 1 } }, (obj) => {
@@ -45,6 +105,7 @@ describe('Object', () => {
         return 'no';
       })
       .exhaustive();
+
     expect(res).toEqual('yes');
   });
 
@@ -64,7 +125,7 @@ describe('Object', () => {
     expect(res).toEqual('yes');
   });
 
-  it('should match object with optional properties', () => {
+  it('should properly match an object against the P.object pattern, even with optional properties', () => {
     const res = match({ x: 1 })
       .with(P.object.empty(), (obj) => {
         type t = Expect<Equal<typeof obj, { readonly x: 1; }>>;