Skip to content
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

Debug render-phase side effects in strict-mode for DEV #12089

Closed
bvaughn opened this issue Jan 24, 2018 · 4 comments · Fixed by #12094
Closed

Debug render-phase side effects in strict-mode for DEV #12089

bvaughn opened this issue Jan 24, 2018 · 4 comments · Fixed by #12094
Assignees

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Jan 24, 2018

Relates to PR #12083

Currently we double-invoke component constructor, render, static getDerivedStateFromProps and setState reducer functions if the debugRenderPhaseSideEffects feature flag is on. We should also do this if we're inside of a StrictMode subtree.

However, it is likely that the new StrictMode element will be present in production apps, so I believe it is important that it not negatively impact performance.

Strict warnings (like all warnings) only impact dev mode, but up until now the double-invoked lifecycles impact both dev and prod. This is actually a good thing at Facebook, since it helps us identify potential bugs in production and we can easily limit it to only impact developers, but I think it would complicate the story for external users because of the negative performance impact.

I propose that we continue to observe the debugRenderPhaseSideEffects flag for dev and prod (since this impacts Facebook only) but only double-invoke for "strict" mode in dev.

Enabling this behavior greatly complicates our incremental rendering tests (see below so I will also be converting these to be internal so they can explicitly override the feature-flag before running. I think this is okay since the tests that break all use the noop-renderer which is not published to NPM.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 25, 2018

Will pick this up after PR #12084 lands.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 25, 2018

This has a broader impact on some of our tests than I'd like. For example, it makes tests like ReactIncrementalUpdates-test and ReactIncremental-test and ReactIncrementalSideEffects-test harder to follow.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 25, 2018

I think I'm going to need to disable this for tests, at least by default. Some of our tests are just too hard to follow otherwise.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 25, 2018

I think the right thing to do for this feature is also to disable it for production mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant