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

Podcast Player - make feed fetch call cancellable and cancel on Block removal. #15228

Merged

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Apr 1, 2020

Currently the Podcast Player Block uses apiFetch to trigger a Ajax request for the feed data. Once it [the fetch promise] resolves then state and attributes updates are triggered.

If, whilst the network request is in progress, the Block is "unmounted" then the console will show an error along the lines of

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.

This is because the .then handler on the fetch still triggers even though the Block no longer exists. As the handler updates state and attributes the console triggers the above error regarding state updates on unmounted components.

To fix this we make the apiFetch call cancell-able (via a wrapper util) and then ensure we cancel and do not update state if the Block has been unmounted.

Fixes #15214

Changes proposed in this Pull Request:

  • Wrap apiFetch to make it possible to cancel it.
  • Add ref to track cancellable promise instance.
  • Add useEffect to .cancel() the fetch promise if the Block unmounts.
  • Add conditional to the .catch() handler to check for .isCancelled and terminate handler execution early to avoid state updates.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Fixes existing bug in Podcast Player Block

Testing instructions:

Annoyingly there seems to be a bug in Gutenberg with the Popover component. Therefore when "removing" the BLock below you will need to click to place the cursor in the editor immediate following the Block and then hit the BACKSPACE key to remove the block. Removing using the dropdown many on the Block itself will trigger a warning which is unrelated to this Block.

On Feature Branch

  • Insert Podcast Block.
  • In Network Tools switch to "Slow 3G mode".
  • Add a valid feed URL and submit.
  • Immediately (before the fetch request resolves) delete the Block (see notes above).
  • See error in console relating to state updates on unmounted component.

On This Branch

  • Insert Podcast Block.
  • In Network Tools switch to "Slow 3G mode".
  • Add a valid feed URL and submit.
  • Immediately (before the fetch request resolves) delete the Block (see notes above).
  • See no errors.

If you enable debug mode you will also see a notice informing you that:

Block was unmounted during fetch

Proposed changelog entry for your changes:

  • Fix React error regarding state updates on unmounted component if Block is removed during the fetching of the Podcast feed.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello getdave! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D41206-code before merging this PR. Thank you!

@getdave getdave linked an issue Apr 1, 2020 that may be closed by this pull request
@jetpackbot
Copy link

jetpackbot commented Apr 1, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 173c8d5

Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

I left only one comment about checking the isCancel state. Maybe it doesn't make sense, though.

Tested and it works as expected.

image

Nice improvement, Dave. 💯

We can wait for more feedback from other folks.

@retrofox retrofox requested a review from a team April 1, 2020 12:55
@matticbot
Copy link
Contributor

getdave, Your synced wpcom patch D41206-code has been updated.

@matticbot
Copy link
Contributor

getdave, Your synced wpcom patch D41206-code has been updated.

@getdave
Copy link
Contributor Author

getdave commented Apr 1, 2020

As this is merging into the feature branch we won't wait on Jetpack Crew.

@getdave getdave merged commit c67575e into feature/podcast-player Apr 1, 2020
@getdave getdave deleted the fix/block-error-on-unmount-during-fetch branch April 1, 2020 13:37
brbrr pushed a commit that referenced this pull request Apr 2, 2020
… removal. (#15228)

* Make fetch call cancellable and cancel on unmount.

* Use optional chaining to reduce complexity

Seems to have been added to Calypso build here

https://github.com/Automattic/wp-calypso/blob/c3608b5c2b656cad1209f52b0e2229838c2592ea/packages/calypso-build/CHANGELOG.md#510

…so should be safe to used as it will be polyfilled.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

* Fix to also exit early on happy .then() path if promise has been cancelled

Kudos to #15228 (comment)
obenland added a commit that referenced this pull request Apr 3, 2020
* Force refetch of feed when identical URL is submitted #15213 (#15219)

* [not verified] Force refetch of feed when identical URL is submitted

* [not verified] Outsource context through ticket link

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Podcast Player: Rename "episode" to "track" for consistency (#15211)

* [not verified] Rename 'episode' to 'track'

* [not verified] Prefix header class names to avoid accidental overrides

* [not verified] Replace render ternaries with &&

* [not verified] Increase image selector specificity

* Simplify podcast title selector
+ add target & rel attributes if is a link

* [not verified] A few more class name changes before scoping

* Rename "cover" class name to "current-track-info"

* Add class to the cover img element

* Make sure &&s don't render any extras when falsy

* [not verified] use audio element from me.js (#15215)

* Podcast Player: Scope podcast player styles for consistency (#15201)

* [not verified] Namespace styles with parent block class

* [not verified] Fix colors inconsistency with master
...with a bonus fix for inactive link colors! \o/

* Podcast Player - make feed fetch call cancellable and cancel on Block removal. (#15228)

* Make fetch call cancellable and cancel on unmount.

* Use optional chaining to reduce complexity

Seems to have been added to Calypso build here

https://github.com/Automattic/wp-calypso/blob/c3608b5c2b656cad1209f52b0e2229838c2592ea/packages/calypso-build/CHANGELOG.md#510

…so should be safe to used as it will be polyfilled.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

* Fix to also exit early on happy .then() path if promise has been cancelled

Kudos to #15228 (comment)

* Add interaction overlay (#15226)

* Update/podcast header ss rendering (#15221)

* podcast-player: add template render function

* podcast-player: add header template

* podcast-player: render podcast title

* [not verified] Fix to use dedicated templates

* podcast-player: fix sniffer errors. esc values. rename vars

* podcast-player: add disabling sniffer rule doc

* Update extensions/blocks/podcast-player/podcast-player.php

Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>

* podcast-player: rename render data param name

* podcast-player: improve doc

Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>

* Podcast Player: format active track in SS (#15230)

* podcast-player: add playlist-track template

* podcast-player: set actve track in SS

* podcast-player: format the actve track in SS

* podcast-player: fix adding `is-active` css Class

* podcast-player: define track vars

* podcast-player: don't create var needlessly

* podcast-player: simplify applying classes. Porps to @obenland

* podcast-player: update aria according on #15207

* Fix invalid HTML for header cover images

Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>

* Podcast Player Accessibility and Interactions (#15207)

* Uses aria-current to indicate current track
* Announcement if track has error.
* Announces loadings state and new title and description
* Announces paused state
* Adds headings and description to playlist
* Adds visually hidden episode title for link context
* Adds aria-labelledby and aria-describedby to playlist to make it be announced as a group with context

* Podcast Player: Set colors to podcast title and description, in server-side. (#15243)

* [not verified] [not verified] podcast-player: apply primary color to title

* podcast-player: apply color to the podcast title

* podcast-player: appluy color to description

* podcast-player: remove extra styles

* podcast-player: fix CSS silly mistake :-/

* Cleaned up repeated color css rules for .jetpack-podcast-player__podcast-title and apply the color even if it's not a link

* podcast-player: simplify applying color to desc

* podcast-plater: remove __container CSS class

Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>

* Add a couple of common keywords

Props @joelwills.

* Podcast Player: truncate podcast description if too long (#15253)

* Podcast Player: change error notice text when user is trying to embed a Spotify hosted podcast (#15254)

* Podcast Player: change error notice text when user is trying to embed a Spotify hosted podcast

* Add linting exemption

* Podcast Player: Document template variables (#15250)

* Update/podcast colors client side (#15249)

* podcast-player: pass colors to children components

* [not verified] podcast-player: set color in podcast title

* [not verified] podcast-player: set podcast title color

* Podcast Player: Add loading and default states (#15234)

* default state style

* add player placeholder

* fix loading bar width

* colocate default styles

* add explanations for 'magic' values

* Remove unnecessary units (and selector)

* use proper escaping for context

* add is_array check for

* use empty check instead of isset

* fix text bug in IE11

* simplify formatting of the error message

* edit: set use callback dependencies properly

* reset vertical padding for player section element

* Add opening quote

Props @marekhrabe

Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Bart Kalisz <bartlomiej.kalisz@gmail.com>
Co-authored-by: Marek Hrabe <marekhrabe@me.com>
Co-authored-by: Damián Suárez <rdsuarez@gmail.com>
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Michael P. Pfeiffer <frontdevde@users.noreply.github.com>
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.

Podcast Player: error if Block unmounts whilst fetching feed.
5 participants