ember-decorators / argument

Decorators for Component and Object arguments in Ember
MIT License
30 stars 18 forks source link

fix: handle null target instances passed to getPropertyDescriptor #99

Closed willviles closed 5 years ago

willviles commented 5 years ago

This PR seeks to fix https://github.com/ember-decorators/argument/issues/98.

-private/utils/object.js#L10 was erroring because target passed to getPropertyDescriptor was in some cases null, not undefined.

export function getPropertyDescriptor(target, property) {
  if (target === undefined) return;
  // target = null...
  return (
    Object.getOwnPropertyDescriptor(target, property) ||
    getPropertyDescriptor(Object.getPrototypeOf(target), property)
  );
}
alexlafroscia commented 5 years ago

Thanks for submitting a PR for this! I actually came online just now to take a pass at solving it, but approving a PR is always better 😁

Did you try linking this locally and find that it solved the problem for you?

artemgurzhii commented 5 years ago

@alexlafroscia The problem I've described in the https://github.com/ember-decorators/argument/issues/98 is resolved with this PR, everything is working great.

@willviles Thanks!

willviles commented 5 years ago

@alexlafroscia yep, linked and it's removed all errors 👍 Not entirely sure whether this PR is the best possible solution, as it's just firefighting methodology. But in the mean time, it solves the problem!

alexlafroscia commented 5 years ago

That's OK as far as I'm concerned -- I'm unsure why I haven't come across this case where the prototype is null in my own codebases that are using this package without error right now.

Are you transpiling away native classes? We ship them in our codebase -- maybe that's the difference?

willviles commented 5 years ago

Yeah I'm shipping ES5, though I've just tried 1.0.0-beta.2 without transpiling native classes and the problem persists 🤷‍♂️

willviles commented 5 years ago

Out of interest, should default values still be respected when using the optional type? Here's what I'm getting:

// component.js
@argument('string')
foo = 'defaultValue'
{{! parent-template.hbs}}
<Component />
{{! component-template.hbs}}
{{log foo}} {{! => undefined}}
artemgurzhii commented 5 years ago

@willviles Same for me, I'll try to come up during this week with all the problems I'm having. I'm using es classes with angle bracket invocation syntax and sometimes I'm receiving unexpected errors(probably not because of angle invocation syntax feature, but I've not tried with the regular way)