Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

fixing inheritance patching #583

Merged
merged 13 commits into from
Oct 22, 2018
Merged

fixing inheritance patching #583

merged 13 commits into from
Oct 22, 2018

Conversation

xaviergonz
Copy link
Contributor

@xaviergonz xaviergonz commented Oct 19, 2018

Fixes #581

Basically it goes back to the "non-optimized" property definition that was at the beginning, since that one works well with inheritance, while the optimized version does not due to the reusage of variables for the instance, the prototype, the possible prototype of the prototype etc.

Either way I don't think it is a big deal since each class is patched only once, and only per class, not per instance.

@xaviergonz
Copy link
Contributor Author

ah, includes previous fix for recursive calls, that one was independent from inheritance

@xaviergonz
Copy link
Contributor Author

and includes unit test for inheritance, for cases where:
(given Base and C, where C extends Base)

  • none is observer
  • Base is observer
  • C is observer
  • C and base are observer

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Oct 19, 2018

Actually even improved it so recursive calls are actually made rather than stopped altogether, but still ensures that mixins are only executed once after all the recursive calls are finished.

I'm confident this new implementation should be much more resiliant

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Oct 20, 2018

@mweststrate pretty much done, now even supporting weird use cases like a base class that uses a prototype method and the extended class uses an arrow function that calls super on the prototype method

@xaviergonz
Copy link
Contributor Author

Btw, the define a property when a property is set in another "this" is basically how JS does things normally internally.

From https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_prototype_chain

Setting a property to an object creates an own property. The only exception to the getting and setting behavior rules is when there is an inherited property with a getter or a setter.

Example:

function FC() { this.a = 10;};
FC.prototype.a = 5;
FC.prototype.b = 10;
const fc = new FC();
Object.getOwnPropertyDescriptor(fc, "a") // some descriptor
Object.getOwnPropertyDescriptor(fc, "b") // undefined
fc.b = 20
Object.getOwnPropertyDescriptor(fc, "b") // some descriptor

@mweststrate
Copy link
Member

@xaviergonz looks solid, good tests, but also complicated. Do you feel comfortable about it or do you fear more potential edge cases? If so, I'm still not against classic monkey patching, despite breaking arrow function lifecycles.

@xaviergonz
Copy link
Contributor Author

pretty confident, had a few days to think about it :)

@xaviergonz
Copy link
Contributor Author

and well, it is 90 lines of code if you ignore the newSymbol and imports, so it is not like super complex

@mweststrate mweststrate merged commit 17056a4 into master Oct 22, 2018
@danielkcz danielkcz deleted the fix-patching-2 branch June 10, 2019 11:37
@github-actions github-actions bot mentioned this pull request Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants