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

PRO-6477 undhandled promise errors #4700

Merged
merged 17 commits into from
Sep 3, 2024
Merged

Conversation

myovchev
Copy link
Contributor

@myovchev myovchev commented Aug 26, 2024

Summary

Cancel debouncers, guard against unhandled promise rejections and potential memory leaks.

What are the specific steps to test this change?

No 'canceled' promise rejections. No UI regressions.

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Copy link

linear bot commented Aug 26, 2024

Copy link
Contributor

@ValJed ValJed left a comment

Choose a reason for hiding this comment

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

Mainly questions.
One point, not sure why we need to spawn empty return all around on methods that has been canceled, we should find another pattern if it's not enough, like not mutating state in the debounced functions if it's a solution.

Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

See also my post in product-development re: a solution on top of this to do away with all the bookkeeping in components etc (it should reuse your work so far within the debouncer).

boutell and others added 3 commits August 29, 2024 13:12
* WIP on if not canceled approach

* no need for finally block

* Bugs and testing

* fix the skipDelay

* capture the timer

* Finish replace debounce async

---------

Co-authored-by: Miro Yovchev <[email protected]>
ValJed
ValJed previously requested changes Aug 29, 2024
Copy link
Contributor

@ValJed ValJed left a comment

Choose a reason for hiding this comment

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

just a point about checking manually this.mounted.

@ValJed ValJed requested review from ValJed and removed request for ValJed August 29, 2024 15:18
@ValJed ValJed dismissed their stale review August 29, 2024 15:19

Not available

@myovchev myovchev merged commit 6d6a109 into main Sep 3, 2024
8 checks passed
@myovchev myovchev deleted the pro-6477-undhandled-promise-errors branch September 3, 2024 13:58
haroun added a commit that referenced this pull request Sep 3, 2024
* main:
  PRO-6477 undhandled promise errors (#4700)
  fix slat order (#4710)
  make apostoggle accessible (#4708)
  Improve context menu positions (#4706)
  remove focus blockers in page relationship editor (#4702)
haroun added a commit to haroun/apostrophe that referenced this pull request Sep 16, 2024
* upstream/main: (77 commits)
  PRO-6578: auto import of inline images (apostrophecms#4723)
  Inline array improvements (apostrophecms#4671)
  take credit for the fix as well as the "change" (apostrophecms#4725)
  PRO-6576 fix default values for object fields in widgets and generally unify newInstance implementations (apostrophecms#4722)
  release 4.7.0 (apostrophecms#4717)
  allows to render widgets in published mode (for non localized with pu… (apostrophecms#4709)
  fix subfield imports (it was a bug in fetchRelationships: false) (apostrophecms#4715)
  Add replaces configuration for editor context menus (apostrophecms#4714)
  bump uploadfs dep (apostrophecms#4712)
  add batch operations to pages (apostrophecms#4684)
  PRO-6477 undhandled promise errors (apostrophecms#4700)
  fix slat order (apostrophecms#4710)
  make apostoggle accessible (apostrophecms#4708)
  Improve context menu positions (apostrophecms#4706)
  remove focus blockers in page relationship editor (apostrophecms#4702)
  Release 4.6.1 mergeback (apostrophecms#4703)
  remove z-index bump on area schema fields, boost menu index (apostrophecms#4699)
  Pro 6345 fix deprecated sass (apostrophecms#4640)
  Keep widget focused when menu is open (apostrophecms#4695)
  skipReplace for changeDocIds (apostrophecms#4694)
  ...
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 this pull request may close these issues.

3 participants