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

Incorrect association of props with owner and factory in FactoryManager.create? #20023

Closed
chriskrycho opened this issue Mar 16, 2022 · 0 comments · Fixed by #20024
Closed

Incorrect association of props with owner and factory in FactoryManager.create? #20023

chriskrycho opened this issue Mar 16, 2022 · 0 comments · Fixed by #20024

Comments

@chriskrycho
Copy link
Contributor

chriskrycho commented Mar 16, 2022

This code is either wrong or quite confusing:

let props = {};
setOwner(props, container.owner!);
setFactoryFor(props, this);
if (options !== undefined) {
props = Object.assign({}, props, options);
}

It appears to be assuming that the setOwner and setFactoryFor will continue to work for props after it is rebound on line 512—but they do not, because it is a completely new object. If I'm reading this method correctly, the original, empty POJO will simply be GC'd, since it's unused after the end of the method.

This has been this way for quite some time, but the cleanups for Ember v4 made it much more apparent.

I believe the correct update here is to change the entire chunk to simply be:

let props = options ? { ...options } : {};
setOwner(props, container.owner!);
setFactoryFor(props, this);

That would maintain the object identity and therefore make owner and factory lookups for this props object continue to work correctly.

chriskrycho added a commit that referenced this issue Mar 16, 2022
Previously, we stomped the `props` binding with an `Object.assign()`,
which meant that the original empty props object would get GC'd after
the end of the method and the item passed into the class created at the
end of the `FactoryManager.create` call would be a *different* object,
which does *not* have the factory or owner associations.

Fixes #20023
chriskrycho added a commit that referenced this issue Mar 16, 2022
Previously, we stomped the `props` binding with an `Object.assign()`,
which meant that the original empty props object would get GC'd after
the end of the method and the item passed into the class created at the
end of the `FactoryManager.create` call would be a *different* object,
which does *not* have the factory or owner associations.

Fixes #20023
kategengler pushed a commit that referenced this issue Mar 21, 2022
Previously, we stomped the `props` binding with an `Object.assign()`,
which meant that the original empty props object would get GC'd after
the end of the method and the item passed into the class created at the
end of the `FactoryManager.create` call would be a *different* object,
which does *not* have the factory or owner associations.

Fixes #20023

(cherry picked from commit d79edb3)
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 a pull request may close this issue.

1 participant