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

Fixed crash on categories collapse, document sidebar close, resize from desktop to mobile #5640

Merged
merged 1 commit into from Mar 15, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Mar 15, 2018

In HierarchicalTermSelector we were not assigning the promise to this.fetchRequest as componentWillUnmount expected.
So each time componentWillUnmount was called the app crashed.

Fixes: #5637

How Has This Been Tested?

Verify it is possible to open and collapse categories panel without any problem.
Verify we can close sidebar with/without categories collapsed, and verify closing sidebar when resizing to mobile also works as expected.
Try to add a category as it generates a different request and see it is still possible to collapse the panel.

@jorgefilipecosta jorgefilipecosta self-assigned this Mar 15, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Mar 15, 2018

@jorgefilipecosta jorgefilipecosta changed the title from Fixed crash on categories collapse. to Fixed crash on categories collapse and on document sidebar close. Mar 15, 2018

@jorgefilipecosta jorgefilipecosta changed the title from Fixed crash on categories collapse and on document sidebar close. to Fixed crash on categories collapse, document sidebar close, resize from desktop to mobile Mar 15, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Mar 15, 2018

Member

Expected to have been caused by #5450 (cc @mkaz). Which means, based on the intention of allowing this to more easily allow wp.apiRequest with promise, means that we'll have the same issue for that use-case, since (as may be known), promises are not cancelable, and Promise#abort does not exist.

Really, it speaks to a bigger problem of this approach to making network requests within a component. One which I'd hope will become resolved with changes in direction like #5219

Member

aduth commented Mar 15, 2018

Expected to have been caused by #5450 (cc @mkaz). Which means, based on the intention of allowing this to more easily allow wp.apiRequest with promise, means that we'll have the same issue for that use-case, since (as may be known), promises are not cancelable, and Promise#abort does not exist.

Really, it speaks to a bigger problem of this approach to making network requests within a component. One which I'd hope will become resolved with changes in direction like #5219

@aduth

aduth approved these changes Mar 15, 2018

👍

Seems we could also consider unsetting this.fetchRequest when the request is completed, so we're not needlessly causing abort (not that I expect it has much overhead).

Fixed crash on categories collapse. In HierarchicalTermSelector we we…
…re not assigning the promise to this.fetchRequest as componentWillUnmount expected.

@jorgefilipecosta jorgefilipecosta merged commit 41f7feb into master Mar 15, 2018

2 checks passed

codecov/project 40.46% (-0.03%) compared to e4d58ec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/fixed-crash-on-categories-collapse branch Mar 15, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Mar 15, 2018

Member

Thank you for the review @aduth, I applied your suggestion of unsetting the variable when the requests are finished.

Member

jorgefilipecosta commented Mar 15, 2018

Thank you for the review @aduth, I applied your suggestion of unsetting the variable when the requests are finished.

@mkaz

This comment has been minimized.

Show comment
Hide comment
@mkaz

mkaz Mar 15, 2018

Member

@aduth - yeh, I had to add in checks for .abort() existing to avoid the errors.

Member

mkaz commented Mar 15, 2018

@aduth - yeh, I had to add in checks for .abort() existing to avoid the errors.

@mkaz mkaz referenced this pull request Apr 5, 2018

Merged

Add a few checks to confirm functions exists before calling #5988

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment