-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Pass prevContext param to componentDidUpdate #8631
Conversation
Sorry. We explicitly did not choose this strategy for the reason that it doesn't make sense to add to everything when only a few places change and it can be recreated while traversing anyway. |
I've updated this PR description with some more context based on our face-to-face chat about this. |
Most of these appear to be copies of React, type definitions, and a few one-off projects with no community activity that use context directly despite us explicitly saying in the docs that it’s not supported. I wouldn’t worry too much about them because updates are already broken for context. Edit: this is getting some traffic so I want to point out that if you relied on this feature, the “fix” to get it back is three lines of code. The benefit of dropping it is that it was broken in the first place (so if you relied on it, your program was likely buggy) and that it introduced an unnecessary memory cost for all components when only a tiny minority used it. |
518b194
to
ec855f5
Compare
ec855f5
to
d9aea00
Compare
"But there still exist some problems" — is this ready to get merged or do you want to add failing cases? |
Hey Dan. 😄 I temporarily set this down because of the holidays. However I also kind of wanted to wait until others on the team weighed in regarding the 3 options above. Do you have any thoughts or preferences? Sebastian and I chatted about this for a good hour or two on Thursday and it was not clear at the end of that talk which option was most appropriate. I thought this PR was worth considering because it fixes a backwards compatibility problem. I didn't realize the was such a big unopened question around it. As for the issues hinted at- there are some problems with context|props being stale if an update is bailed out on midway (after render but before commit) and then later updated again by a higher priority change. I will write a failing test or two for that on Tuesday even if we don't fix it right away. (Started writing one already but haven't finished yet.) 😁 Happy holidays! |
Yea, no rush, just curious. |
No worries. I should have left a clearer status on the PR before heading out. I was starting to feel bike-sheddy about this on Thursday and I was hoping a few days away from it might lend some clarity. |
336b05d
to
d3dbb13
Compare
d3dbb13
to
de2356f
Compare
Note to self: PR #8655 has implications on this that probably pushes us toward either option B or not passing I haven't wrapped my head fully around this yet but I think that if it's possible for |
I've been thinking more about the lifecycle methods and specific concerns about First some assumptions I have:
Assuming the above assumptions are correct, I have some concerns about fiber and I think it's not controversial to say that side-effect things (eg DOM manipulation, network requests) should only be done in What about other instance attributes? Using react-virtualized as an example again- I store some derived data on the instance. This data is outside of React's management/awareness and so it won't be reset when a Fiber is bailed out on. I think that PR #8655 mitigates this by explicitly resetting But what about instance attributes that are derived from
I believe @sebmarkbage has some ideas/plans for Update: Chatted in meatspace with @acdlite about this. He pointed out another existing stack limitation with fiber WRT I think I'm now leaning toward option 1 above (removing the |
I want to expand on my concern about instance attributes a little more while I'm thinking about it. I think it's unsafe to do anything with side effects (even setting or updating "private" instance attributes) in (There may be operations that should only be done once- when an input changes from X to Y. Because we might call This is also a potential problem if we store anything references to non-primitive types in @sebmarkbage I'd like to chat with you about this next week if you're available. 😄 |
Ping @sebmarkbage. Now that you're back, can we chat about this? |
This makes use of an expando property (__reactInternalPrevContext) on the stateNode (instance). A property on the instance was used instead of a Fiber attribute because: 1) Conditional fields on fibers would break monomorphism. 2) Adding to all fibers would bloat types that don't use context.
This affects Stack and Fiber. I've updated impacted tests as well. This may not be the direction we choose to go either. More discussion is still needed about the future of context...
* Update changelog for unreleased 16.0 changes (#10730) * First shot at updating changelog for 16.0 **what is the change?:** Added an 'unreleased' section to the changelog with info from #10294 **why make this change?:** To get things set for the 16.0 release. **test plan:** Visual inspection **issue:** #8854 * Fix typos and formatting errors in changelog * Add requestAnimationFrame and remove "New helpful warnings" **what is the change?:** In response to helpful code review - - Add mention of dependency on `requestAnimationFrame` and need to polyfill that as well as `Map` and `Set` - Remove "New helpful warnings" section; it was incomplete, and there are so many new and updated warnings that it might not be reasonable to cover them in the changelog. **why make this change?:** Accuracy **test plan:** Visual inspection **issue:** issue #8854 * Improve wording * Improve wording and fix missing links * Add backticks to file names & code; wording tweak * Break "Major Changes" into "New Feature" and "Breaking Changes" * Add server side render changes to 16.0 changelog * Change gist link from mine to gaearons * Add note about returning fragments * fix misc nits * Misc. formatting/wording fixes to changelog **what is the change?:** Thanks to the kind code review comments of @gaearon and @nhunzaker we have - removed the non-deterministic bold styling on some bullet points - improved wording of the bullet points for portals, attribute whitelist changes, and server rendering changes - Add note about error boundaries including a breaking change to error handling behavior. - punctuation and capitalization fixes **why make this change?:** Clarity and correctness **test plan:** Visual inspection **issue:** #8854 * fix broken link * Fixes #9667: Updated createTextInstance to create the text node on correct document (#10723) * Record sizes * Add a changelog for elements having the same key (#10811) * Add a changelog for elements having the same key * Reword * Markdown fixs on "DOM Attributes in React 16" post (#10816) * Include tag name into the table snapshot (#10818) * Update DOM warning wording and link (#10819) * Update DOM warning wording and link * Consistently use "Invalid" for known misspellings * Update license headers BSD+Patents -> MIT Did find and replace in TextMate. ``` find: (?:( \*)( ))?Copyright (?:\(c\) )?(\d{4})\b.+Facebook[\s\S]+(?:this source tree|the same directory)\.$ replace: $1$2Copyright (c) $3-present, Facebook, Inc.\n$1\n$1$2This source code is licensed under the MIT license found in the\n$1$2LICENSE file in the root directory of this source tree. ``` * Change license and remove references to PATENTS Only remaining references: ``` docs/_posts/2014-10-28-react-v0.12.md 51:You can read the full text of the [LICENSE](https://github.com/facebook/react/blob/master/LICENSE) and [`PATENTS`](https://github.com/facebook/react/blob/master/PATENTS) files on GitHub. docs/_posts/2015-04-17-react-native-v0.4.md 20:* **Patent Grant**: Many of you had concerns and questions around the PATENTS file. We pushed [a new version of the grant](https://code.facebook.com/posts/1639473982937255/updating-our-open-source-patent-grant/). src/__mocks__/vendor/third_party/WebComponents.js 8: * subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt ``` * Version bumps to use MIT license * Add ReactTestRenderer documentations (#10692) * Add ReactTestRenderer documentations * Add TestRenderer documentations * TestRenderer is not experiment * Add a link for jsdom * Use ES Modules syntax * Twaek * Add a Link component * Use Jest assertions * Move a documentation for createNodeMock to Idea section * Renamed * Tweak * Rename * More explicit * Add a usecase for createNodeMock * Add changelog for 15.6.2 * Add 15.6.2 blog post to master * Add Nate to authors on master * Bump object-assign patch range to match main package.json * Flow should ignore node_modules/create-react-class * Update error codes * Update CHANGELOG for React 16 * v16.0.0 * Doc updates for React 16 + blog post (#10824) * Doc updates for React 16 + blog post * Add link to Sophie's post * Fix React links on the website (#10837) * Fix React links on the website * Fix code editor * Fix code editor, attempt 2 * Doc change for prevContext removal in CDU (#10836) * Doc change for prevContext removal in CDU Ref: #8631 * Minor rewording * Fix note formatting * React.createPortal is not a function (#10843) * Update Portals Documentation (#10840) * Update Portals Documentation Correct some grammar to be more explicit and clear. Update example CodePen to better match code found in documentation. Update code example styles to match other code examples (ie. 'State and Lifecycle', 'Handling Events'). * Clean up comment to be accurate to example There was a small comment overlooked when reviewing the documentation. This fixes it to be accurate to the example as well as grammatically correct. * Update portals.md * More fixes * Update name of property initializer proposal (#10812) The proposal for property initializers is called [Public Class Fields](https://tc39.github.io/proposal-class-public-fields/) now (part of the combined [Class Fields](https://github.com/tc39/proposal-class-fields) proposal). * Fix portal link (#10845) * Update docs for React 16 (#10846) * Minor doc edit * Rename urls
See this comment for my latest thoughts about this issue.
This PR resolves the issue of the missing
prevContext
param tocomponentDidUpdate
calls.Note that this approach is support option 1 below.
Options Considered
It seems like we have a high-level decision to make regarding how and if we support the
prevContext
parameter going forward. Options include:ReactContextValidator-test
test to assert that theprevContext
parameter is not passed and update to Stack to mirror this behavior.__reactInternalPrevContext
). Adding to the instance avoids bloating Fibers for all of the types that don't need this property and keeps our Fiber-consuming functions monomorphic.stateNode
to store a wrapper object around instance and previous context for the case ofClassComponent
s. This prevents all non-ClassComponent
fibers from growing while also keeping fiber-consuming methods monomorphic. Since this only impactsClassComponent
it should hopefully introduce minimal forking.Regarding continued support: At this time it does not seem like this parameter is being used by React components within Facebook but a quick Github search seems to suggest the parameter is being used externally (eg 1, 2, 3, etc.).