-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Implemented Profiler onCommit() and onPostCommit() hooks #17910
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 858c870:
|
Details of bundled changes.Comparing: e1c7e65...858c870 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
Details of bundled changes.Comparing: e1c7e65...858c870 react-art
react-dom
react-native-renderer
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Size changes (experimental) |
30358f6
to
26157b4
Compare
I have rebased this on top of PR #17947 to pull in the various passive effect fixes. |
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.
Could you rebase this on top of master so build bot can run? Just to confirm that this should have no bundle size impact.
Yes. I've been working on this off and on all afternoon. The recent passive effects changes require changing some of the interaction tracing tests (which I'm working on). Should have it ready to review by tomorrow morning. |
26157b4
to
470e4ef
Compare
@sebmarkbage rebase complete. Changes to the profiler tests look larger than they are, because I replaced all of the I also updated tests to run with the deferred passive effects flag on and off, so be sure to use the |
while (parentFiber !== null) { | ||
if (parentFiber.tag === Profiler) { | ||
const parentStateNode = parentFiber.stateNode; | ||
if (parentStateNode !== null) { |
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.
This check is something that we wouldn't need to do if we eagerly flushed passive destroy functions during unmount.
describe('onRender callback', () => { | ||
beforeEach(() => { | ||
jest.resetModules(); | ||
[true, false].forEach(deferPassiveEffectCleanupDuringUnmount => { |
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.
This method of testing feature flags is controversial. I think it's nice to have the extra coverage, and these tests are already very verbose so the idea of duplicating them is not very appealing- but I don't want to die on this hill. I'll remove this and just test one of the flag settings if that's the team preference.
470e4ef
to
7c7c918
Compare
FYI @sebmarkbage I've added a new feature flag (off by default) called |
7c7c918
to
858c870
Compare
This PR adds two new callbacks to the Profiler,
onCommit
andonPostCommit
, as proposed in RFC 139 (reactjs/rfcs#139).When reviewing, use the
?w=1
feature flag to ignore whitespace