alexeyraspopov / dataclass

Data classes for TypeScript & JavaScript
https://dataclass.js.org
ISC License
181 stars 6 forks source link

Skip existant getters when defining immutable properties #3

Closed ghost closed 7 years ago

ghost commented 7 years ago

Hi. A little proposal. When a class extends from Record and has some getter functions (via get ... syntax), these properties get readded in the Record constructor. And these getters become bound to the old context with default values, completely ignoring custom values. Namely, functions

getAddr() {
    return '@' + this.name;
}
get addr() {
    return '@' + this.name;
}

will return different results if the "name" property was defined in the custom values.

alexeyraspopov commented 7 years ago

@art-mickiewicz, thank you for your contribution!

I like the idea in general. Before merging, could you please explain the logic behind your code? Not sure I can clearly see it. Thanks.

ghost commented 7 years ago

Ok. Function getPropertyDescriptor is needed to get property descriptor (Object.getOwnPropertyDescriptor) of any property on the object regardless where in the hierarchy it is defined, not only "own" properties. And in my case getters end up defined on the parent (prototype). Then I check the descriptor in the loop where all properties are converted to getters. And descriptor containing "get" key means that the property is a getter already, so we just skip it.

alexeyraspopov commented 7 years ago

@art-mickiewicz, correct me if I'm wrong.

I've tried another way to deal with the issue:

for (const key in base) {
+  if (!Object.getOwnPropertyDescriptor(base, key)) continue;
// ...
}

Since we just need to not to overwrite already defined getters, we can skip keys that were not defined as own. What do you think?

ghost commented 7 years ago

Yes, this solution works perfectly. I was just little worried about what would happen if we had a dataclass which inherits from another class. But then I understood that this inheritance is not really possible. So, your solution fits better.

alexeyraspopov commented 7 years ago

I was just little worried about what would happen if we had a dataclass which inherits from another class. But then I understood that this inheritance is not really possible.

Right, I think, even if it was possible, it would not be really useful.

If you're going to update the branch, could you please consider adding at least one test? I can help you with writing it if needed.