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

Ensure lifecycle timers are stopped on errors #7548

Merged
merged 2 commits into from
Aug 24, 2016
Merged

Ensure lifecycle timers are stopped on errors #7548

merged 2 commits into from
Aug 24, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 23, 2016

This fixes #7349 and removes a special case for error boundaries that is now unnecessary.
The perf impact on DEV is minimal because try / finally is separated into a function.

if (inst.componentDidMount) {
if (__DEV__) {
transaction.getReactMountReady().enqueue(invokeComponentDidMountWithTimer, this);
if (__DEV__ && debugID !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that our www pipeline understands this. I think it's really dumb and only look for if (__DEV__) as the previous code used two nested ifs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprisingly, it does:

$ echo 'if (__DEV__ && debugID !== 0) alert(1);' | jsxmin --replace __DEV__:1
if(debugID!==0)alert(1);
$ echo 'if (__DEV__ && debugID !== 0) alert(1);' | jsxmin --replace __DEV__:0
$

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (¯\_(ツ)_/¯)

@vjeux
Copy link
Contributor

vjeux commented Aug 23, 2016

Thanks for keeping up with all my questions, this diff looks good to me, here are the things I'd like fixed before accepting it:

  • Use this._debugID everywhere except for the only place where you need to pass it down and keep the existing if block (you can rename the variable if you want) and would be nice to add a comment above saying why you are doing this weird dance.
  • Embed the this._debugID !== 0 check inside of measureLifeCyclePerf so you don't need to do it everywhere.
  • Remove the context argument altogether instead of passing a confusing null
  • Make the two branches match in the enqueue section

@ghost ghost added the CLA Signed label Aug 23, 2016
@@ -86,6 +86,94 @@ describe('ReactDOMProduction', function() {
expect(container.childNodes.length).toBe(0);
});

it('should call lifecycle methods', function() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this test so we don’t accidentally regress in prod.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 24, 2016

This will also fix #6949.

@sophiebits
Copy link
Collaborator

Looks good as long as this isn't noticeably slower.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 24, 2016

Doesn't seem to be in my testing, extracting try/finally works well (as proven by other libraries like Bluebird).

@zpao zpao modified the milestones: 15.3.2, 15-next Sep 8, 2016
zpao pushed a commit that referenced this pull request Sep 15, 2016
* Ensure lifecycle timers are stopped on errors

Fixes #7349

* Address review feedback and add an extra test

(cherry picked from commit a229cdb)
@beholderrk
Copy link

In developer mode, I don't see any errors in the console when component render, measureLifeCyclePerf catch everything. Is this problem met before?

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 11, 2016

Try/finally blocks can't possibly catch errors. If you don't see errors thrown in your code, this is a problem with your code. For example maybe you are using Promises incorrectly and catch all errors, including errors in your code, and ignore them. This is a common mistake when you call setState() and then use catch() for handing errors. In any case we can't help unless you provide an example reproducing the problem.

@beholderrk
Copy link

Oh sorry. I thought that try/finally as same as try/catch/finally with empty catch block. Thanks!

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

Successfully merging this pull request may close these issues.

Did not expect componentDidMount timer to start while render timer is still in progress for another instance
5 participants