-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
[compiler] Allow inferred non-optional paths when manual deps were optional #30816
[compiler] Allow inferred non-optional paths when manual deps were optional #30816
Conversation
…tional If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…tional If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. ghstack-source-id: 8f34860deafefe48c7a374caa1b55482e571308d Pull Request resolved: #30816
…eps were optional" If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
…eps were optional" If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
…eps were optional" If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. [ghstack-poisoned]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change makes sense intuitively, but I'm not sure if it precisely preserves previous memo behavior.
Take the current test fixture:
function Component(props) {
const data = useMemo(() => {
// actual code is non-optional
return props.items.edges.nodes ?? [];
// deps are optional
}, [props.items?.edges?.nodes]);
return <Foo data={data} />;
}
Let's say the initial render used props = {items: {edges: {nodes: undefined}}}
. The useMemo
would take a dep on [undefined]
, and we get data = []
.
If the next render passes props = {items: undefined}
, the useMemo dependencies don't change (and we would reuse data = [[ previous_result ]]
). With Forget enabled, this would throw.
Just wanted to note that this could technically happen. If we want to preserve precise semantics of existing memo, we might need to use a strict optional
comparison.
Good catch, i'll update this to ensure that the inferred deps exactly match the optionality of the manual deps (will fix-forward though) |
…tional If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation. The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule. ghstack-source-id: 9f7a99ee5f7caa2c2d96f70f360e4320bac3de2d Pull Request resolved: #30816
**breaking change for canary users: Bumps peer dependency of React from `19.0.0-rc-7771d3a7-20240827` to `19.0.0-rc-94e652d5-20240912`** [diff facebook/react@7771d3a7...94e652d5](facebook/react@7771d3a...94e652d) <details> <summary>React upstream changes</summary> - facebook/react#30952 - facebook/react#30950 - facebook/react#30946 - facebook/react#30934 - facebook/react#30947 - facebook/react#30945 - facebook/react#30938 - facebook/react#30936 - facebook/react#30879 - facebook/react#30888 - facebook/react#30931 - facebook/react#30930 - facebook/react#30832 - facebook/react#30929 - facebook/react#30926 - facebook/react#30925 - facebook/react#30905 - facebook/react#30900 - facebook/react#30910 - facebook/react#30906 - facebook/react#30899 - facebook/react#30919 - facebook/react#30708 - facebook/react#30907 - facebook/react#30897 - facebook/react#30896 - facebook/react#30895 - facebook/react#30887 - facebook/react#30889 - facebook/react#30893 - facebook/react#30892 - facebook/react#30891 - facebook/react#30882 - facebook/react#30881 - facebook/react#30870 - facebook/react#30849 - facebook/react#30878 - facebook/react#30865 - facebook/react#30869 - facebook/react#30875 - facebook/react#30800 - facebook/react#30762 - facebook/react#30831 - facebook/react#30866 - facebook/react#30853 - facebook/react#30850 - facebook/react#30847 - facebook/react#30842 - facebook/react#30837 - facebook/react#30848 - facebook/react#30844 - facebook/react#30839 - facebook/react#30802 - facebook/react#30841 - facebook/react#30827 - facebook/react#30826 - facebook/react#30825 - facebook/react#30824 - facebook/react#30840 - facebook/react#30838 - facebook/react#30836 - facebook/react#30819 - facebook/react#30816 - facebook/react#30814 - facebook/react#30813 - facebook/react#30812 - facebook/react#30811 </details> --------- Co-authored-by: vercel-release-bot <infra+release@vercel.com>
Stack from ghstack (oldest at bottom):
If the inferred deps are more precise (non-optional) than the manual deps (optional) it should pass validation.
The other direction also seems like it would be fine - inferring optional deps when the original was non-optional - but for now let's keep the "at least as precise" rule.