Make enableOwnerStacks dynamic #31661
Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 79ddf5b...e06f8b8 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
noahlemen
force-pushed
the
rh/dymanic-owner-stacks
branch
from
43b4f2b to
25b3193
Compare
| flags.enableOwnerStacks | ||
| ? [ | ||
| 'Component uses the legacy childContextTypes API which will soon be removed. Use React.createContext() instead.', | ||
| {withoutStack: true}, |
There was a problem hiding this comment.
this doesn't seem like it should be needed, still working on figuring out what's going on here
There was a problem hiding this comment.
think this may actually be correct. the error below for contextTypes has an owner stack because it is on Foo, which is rendered by Outer. childContextTypes is on Outer, however, which has no owner – it is rendered directly by root.render. If Outer is wrapped with an arbitrary wrapper component, we get a stack here.
There was a problem hiding this comment.
why wasn't this failing before though?
There was a problem hiding this comment.
Looks like a bug in the checker to me. Why do we need to gate only first message?
There was a problem hiding this comment.
@hoxyq see my second comment - it seems there's no "owner" to point to (it would be the root.render call on L338), so there's no stack
@rickhanlonii it wasn't failing before because owner stacks weren't enabled until this PR, right?
There was a problem hiding this comment.
see my second comment - it seems there's no "owner" to point to (it would be the
root.rendercall on L338), so there's no stack
Makes sense, but when enableOwnerStacks is disabled, classic component stacks should be appended, which will be empty in this case as well, but was working well without specifying withoutStack property.
There was a problem hiding this comment.
that's true - it appears that with enableOwnerStacks disabled the component stack here ends up pointing at that anonymous function. not sure if we should expect owner stacks to do the same?
There was a problem hiding this comment.
that's true - it appears that with
enableOwnerStacksdisabled the component stack here ends up pointing at that anonymous function. not sure if we should expect owner stacks to do the same?
oh, now I get it, so non-empty component stack gets appended, if feature flag is disabled, but once its enabled, then the owner stack is empty and its basically a false-positive
thanks for the explanation
There was a problem hiding this comment.
why wasn't this failing before though?
to fully close the loop on this, @rickhanlonii, it seems its just because with enableOwnerStacks prior only enabled in experimental, disableLegacyContext was always enabled
since this test has // @gate !disableLegacyContext, it plainly was never running before when enableOwnerStacks was gated, as there wasn't a permutation of the feature flags where disableLegacyContext was false but enableOwnerStacks was true. enableOwnerStacks was only ever true when disableLegacyContext was already true.
| let captureOwnerStack: ?() => null | string; | ||
| if (enableOwnerStacks) { | ||
| captureOwnerStack = captureOwnerStackImpl; | ||
| } |
There was a problem hiding this comment.
This is sus, why does this need exported here but not from the other ReactServer.js exports?
There was a problem hiding this comment.
in your PR it was added without the flag check, which seemed to break feature detection in the tests. so i added the gating, which corrected it.
you're right though, i hadn't noticed that ReactServer.js doesn't export it at all. looks like just not changing this file at all would be a valid approach too.
There was a problem hiding this comment.
after investigating this a bit more, i think exporting this dynamically based on the feature flag is correct.
in the experimental builds we have a dedicated file for the development builds (ReactServer.experimental.development vs ReactServer.experimental, index.experimental.development vs index.experimental) and only export captureOwnerStack in the development version.
the *.fb files do not have separate development versions, so if we want parity with experimental, we should gate defining the export on __DEV__.
however, if we gate based on just that, we still get some failures. there's another subtle inconsistency - in experimental builds the flag is always enabled. here, we use __VARIANT__, which creates a situation where, if we gate only based on __DEV__ we could export a defined captureOwnerStack but have enableOwnerStacks disabled (eg: when tests are run with --variant=false).
because some of the existing test infrastructure code (1, 2) for handling this operates based on feature detection of captureOwnerStack being defined, this is very consequential for how the tests are ultimately run.
| flags.enableOwnerStacks | ||
| ? [ | ||
| 'Component uses the legacy childContextTypes API which will soon be removed. Use React.createContext() instead.', | ||
| {withoutStack: true}, |
There was a problem hiding this comment.
why wasn't this failing before though?
following up on facebook#31287, fixing tests --------- Co-authored-by: Rick Hanlon <rickhanlonii@fb.com> DiffTrain build for [a496498](facebook@a496498)
following up on facebook#31287, fixing tests --------- Co-authored-by: Rick Hanlon <rickhanlonii@fb.com> DiffTrain build for [a496498](facebook@a496498)
following up on #31287, fixing tests