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

[BUGFIX beta] Correctly associate props with factory and owner in FactoryManager #20024

Merged
merged 1 commit into from Mar 16, 2022

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented 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

Also fixes a bunch of unnecessary type casts.

@rwjblue rwjblue changed the title Correctly associate props with factory and owner in FactoryManager [BUGFIX lts] Correctly associate props with factory and owner in FactoryManager Mar 16, 2022
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

The container changes seem good! Can you revert the routing.ts / router.ts changes (and move those type only things to another PR)?

@chriskrycho
Copy link
Contributor Author

Yes! Thanks for flagging those up; did not mean to include them! Updating momentarily.

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 chriskrycho force-pushed the chriskrycho/fix-FactoryManager.create branch from 0b154e7 to d79edb3 Compare March 16, 2022 19:07
@chriskrycho
Copy link
Contributor Author

Fixed, @rwjblue!

@chriskrycho
Copy link
Contributor Author

I've just checked and I do not think we can backport this one—not only b/c it's not a clean back port, but because it's doesn't work when I do the equivalent refactor there! It also doesn't affect the underlying issue we found in #20025, so it seems fine to me to just land it a fix on master and beta.

@chriskrycho chriskrycho merged commit 4fdb67c into master Mar 16, 2022
@chriskrycho chriskrycho deleted the chriskrycho/fix-FactoryManager.create branch March 16, 2022 19:31
@chriskrycho chriskrycho changed the title [BUGFIX lts] Correctly associate props with factory and owner in FactoryManager [BUGFIX beta] Correctly associate props with factory and owner in FactoryManager Mar 16, 2022
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.

Incorrect association of props with owner and factory in FactoryManager.create?
2 participants