Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] hooks: useSubEnv and useChildSubEnv should apply on prototype chain #1447

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BastienFafchamps
Copy link
Contributor

Before this commit:
When using useSubEnv or useChildSubEnv, only owned properties of the extensions are assigned to the new env.

After this commit:
Properties from the whole prototype chain of the extension are assigned to the env.

…hain

Before this commit:
When using useSubEnv or useChildSubEnv, only owned properties of the
extensions are assigned to the new env.

After this commit:
Properties from the whole prototype chain of the extension are assigned
to the env.
Copy link
Contributor

@sdegueldre sdegueldre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about the reason behind this change. It doesn't seem obvious to me that it should behave this way. The caller can flatten the object before calling use(Child)SubEnv if needed. This seems to encourage patterns that would lead to confusing behaviour, eg useSubEnv(new SomeClass()) where the env will have all the properties and methods of SomeClass but will not be instanceof SomeClass and if the prototype of SomeClass is modified/patched the sub-env will not reflect the changes to the prototype.

Comment on lines +42 to +44
for (const key in descrs) {
descrs[key].configurable = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to do anything?

Comment on lines +39 to +41
while (extension !== Object.prototype) {
const descrs = Object.getOwnPropertyDescriptors(extension);
Object.defineProperties(env, descrs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and because we're doing this while climbing the prototype chain, properties on the prototype will shadow properties on the object instead of the other way around which is undesirable in any case.

@ged-odoo
Copy link
Contributor

ged-odoo commented Jun 1, 2023

Curious about the reason behind this change. It doesn't seem obvious to me that it should behave this way. The caller can flatten the object before calling use(Child)SubEnv if needed.

Not easy to do in some cases. Here, we are in a situation where we want to do useSubEnv(someOtherEnv), but if you flatten the other env, you evaluate immediately all the getters.

@sdegueldre
Copy link
Contributor

if you flatten the other env, you evaluate immediately all the getters
Not if you flatten it in the same way as we do in this PR.

Doing useSubEnv(otherEnv) seems like a very weird use case and I'm curious how you even get in a situation where a component has a hold of 2 environments and wants an actual sub-env. It seems more likely you want to replace the env completely. I've had to do this in a few places in Odoo while migrating legacy code and did it by writing on owl internals __owl__.env and __owl__.childEnv. If this is a use case we want to support I'd rather be in favour of a hook that lets you replace the env wholesale instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants