omermorad / mockingbird

🐦 Decorator Powered TypeScript Library for Creating Mocks
MIT License
87 stars 5 forks source link

bug: using `null` or `undefined` as the `@Mock(() => result)` throws error #138

Open omermorad opened 2 years ago

omermorad commented 2 years ago

If i use null or undefined as the @Mock(() => result), it throws:

    TypeError: Cannot convert undefined or null to object
        at Function.getOwnPropertyNames (<anonymous>)

      at getClassMembers (../../../node_modules/@plumier/reflect/lib/parser.js:87:28)
      at parseMethods (../../../node_modules/@plumier/reflect/lib/parser.js:134:21)
      at parseClassNoCache (../../../node_modules/@plumier/reflect/lib/parser.js:173:18)
      at ../../../node_modules/@plumier/reflect/lib/helpers.js:18:31
      at walkTypeMembers (../../../node_modules/@plumier/reflect/lib/walker.js:35:45)
      at walkTypeMembersRecursive (../../../node_modules/@plumier/reflect/lib/walker.js:54:23)
      at reflectClass (../../../node_modules/@plumier/reflect/lib/reflect.js:15:55)
      at reflectModuleOrClass (../../../node_modules/@plumier/reflect/lib/reflect.js:66:16)
      at ../../../node_modules/@plumier/reflect/lib/helpers.js:18:31

Originally posted by @arthurfiorette in https://github.com/omermorad/mockingbird/issues/135#issuecomment-1017600274

arthurfiorette commented 2 years ago

Sorry for the delay, I was traveling. Did you manage to reproduce this bug?

omermorad commented 2 years ago

Keep on doing that, we need some fresh air sometimes :) I didn't get to it yet, but until I will - if you have a chance to add a snippet it might be helpful!

arthurfiorette commented 2 years ago

Hey @omermorad, It's me again.

This bug is occurring when i use the T | null or T | undefined type.

class Test {
 @Mock(...)
 fine: string;

 @Mock(...)
 alsoFine?: string;

 @Mock(...)
 notFine: string | null;

 @Mock(...)
 alsoNotFine: string | undefined;
}

And that's because with a strict tsconfig, the decorator metadata emitted for these lines are different:

// With fine?: string;
tslib_1.__metadata("design:type", String)

// With notFine: string | null;
tslib_1.__metadata("design:type", Object)

Note that this only occurs with strict: true on the tsconfig.json.

This could be alright if it was trying automatically to find a mock value, but as it was manually specified (not this), I don't think that the metadata should be read and interpreted.

And when it needs to be interpreted, but the generated metadata is Object, it should emit a warning (not an error) or adapt itself like other decorators libraries do.

// type-graphql does manually the type

@Field(() => String) // <--
a: string;

I don't have time to create a 100% reproductible steps in the moment, but this should be sufficient...

omermorad commented 2 years ago

@arthurfiorette what version are you using? Can you try it with 3.0.0-rc.3?

arthurfiorette commented 2 years ago

Yep, 3.0.0-rc.3 works just fine!

arthurfiorette commented 2 years ago

But, is it safe to upgrade? If it is, you should create a stable release (3.0.0)...

omermorad commented 2 years ago

Yes, it's been tested with a new integration test, look here: https://github.com/omermorad/mockingbird/blob/mockingbird%403.0.0-rc.3/packages/parser/test/integration/common/test-classes.ts, those are all actual classes that are being tested (simulates a real situation).

I belive I will release this in the next week, I almost have no time to breathe now because of the new startup I work for :)

ktutnik commented 1 year ago

Hello, Plumier developer here. I come here because I see that the error was related to the @plumier/reflect error.

We currently have some bugs fixed on our newest release v1.1.0, let me know if you having any further issues.

arthurfiorette commented 1 year ago

@omermorad ?