tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
384 stars 27 forks source link

fix(decorators): don't chain the result as it can result in undefined #52

Closed qfl1ck32 closed 1 year ago

qfl1ck32 commented 1 year ago

This is a pretty important bug, in my opinion.

Real use case:

@decorate(ObjectType())
@decorate(Schema())
export class Blameable {
  @decorate(Field(() => String))
  @decorate(Prop())
  createdByUserId: ObjectId;
}

Result:

.../node_modules/@nestjs/graphql/dist/decorators/object-type.decorator.js:24
            name: name || target.name,
                                 ^
TypeError: Cannot read properties of undefined (reading 'name')
    at addObjectTypeMetadata (.../node_modules/@nestjs/graphql/dist/decorators/object-type.decorator.js:24:34)
tannerntannern commented 1 year ago

Thanks for the PR @qfl1ck32. There are a few issues that must be dealt with before I spend time on an in-depth review.

  1. The entire file you changed was reformatted, which is not strictly necessary for the fix you're proposing. The reformatting obscures the meaningful changes and makes the diff hard to read. Furthermore, the reformatting goes against the prevailing code style (e.g., single vs double quotes) which is not something I'm willing to debate. Please only commit the functional changes.
  2. There needs to be some sort of test case demonstrating the intended behavior. The test directory isn't super well-organized right now, but I think a test/regression subdirectory would be a good place for it.

Once these issues are addressed I will take another look.

qfl1ck32 commented 1 year ago

Hey, I have fixed the formatting. For the tests, I don't want to add any kind of other dependency to prove this - i.e., testing this with nestjs' ObjectType decorator, but it's easy to imagine that not all decorators return the class, and for this reason, we can't simply do for ... { class = decorate(class); } - we need to check if the decorator returns the class or not. If it does, we should assign it back to our initial class (even though, to be honest, I don't know exactly why a decorator would return it?)

tannerntannern commented 1 year ago

Thanks for the updates @qfl1ck32. It's much easier to understand what changed now. I actually didn't realize that it wasn't an option for class decorators not to return anything, but it appears they are allowed to. I don't believe this is the case for Python decorators, which is perhaps where this false assumption came from.

Adding a dependency is not required to do a regression test on this behavior, so I'm still going to insist on a test case, but I won't force you to write it. I have a little bit of time this evening to take care of it.

tannerntannern commented 1 year ago

@qfl1ck32 this was released in v6.0.2. I also added a quick test to verify the intended behavior, which is just "class decorators that don't return anything are supported."

Thank you again for catching the bug and improving the library!