Add config version, and invalidate layout on config change #1674
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
Shared with Meta
There was a problem hiding this comment.
This looks good! But I think it introduces subtle bug that breaks layout caching in Fabric, around assigning new config to already created node.
In current Fabric impl, we lazily clone any committed ShadowNodes when we know we may need to write new layout information to them. This currently means new inline Yoga node and Yoga config in the ShadowNode. The new config is associated with cloned node via YGNodeSetConfig, and right now this only dirties the node if meaningful config values have changed.
After this change, if we have new config, derived from previous one, they may be structurally equal, but have a different revision count. So the process of building new config, derived from previous, may still lead to invalidation. Or, in the reverse, a config revision ID is no longer valid if a new config is assigned, and we should clear it (though we do dirty anyways).
I think, we can address this, by adding code around the existing equality check (IIRC in Node::setConfig()), to update kept revision ID to match new config revision ID if they are equal, or clear if not.
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: This is a continuation of the previous PR: facebook#45047 I made the change more generic for allowing any kind of config change to invalidate layout. X-link: facebook/yoga#1674 Differential Revision: D59286992 Pulled By: NickGerleman
…45259) Summary: Pull Request resolved: facebook#45259 This is a continuation of the previous PR: facebook#45047 I made the change more generic for allowing any kind of config change to invalidate layout. X-link: facebook/yoga#1674 Reviewed By: rozele Differential Revision: D59286992 Pulled By: NickGerleman
…1674) Summary: This is a continuation of the previous PR: facebook/react-native#45047 I made the change more generic for allowing any kind of config change to invalidate layout. Pull Request resolved: facebook#1674 Differential Revision: D59286992 Pulled By: NickGerleman
Summary: X-link: facebook/react-native#45259 This is a continuation of the previous PR: facebook/react-native#45047 I made the change more generic for allowing any kind of config change to invalidate layout. Changelog: [Internal] X-link: facebook/yoga#1674 Reviewed By: rozele Differential Revision: D59286992 Pulled By: NickGerleman fbshipit-source-id: f46f35b03d5d9a743b798844ee3e1a02c271ccde
Summary: Pull Request resolved: #45259 This is a continuation of the previous PR: #45047 I made the change more generic for allowing any kind of config change to invalidate layout. Changelog: [Internal] X-link: facebook/yoga#1674 Reviewed By: rozele Differential Revision: D59286992 Pulled By: NickGerleman fbshipit-source-id: f46f35b03d5d9a743b798844ee3e1a02c271ccde
|
@NickGerleman merged this pull request in e4fe14a. |
This is a continuation of the previous PR: facebook/react-native#45047
I made the change more generic for allowing any kind of config change to invalidate layout.