Skip to content

Conversation

@BeksOmega
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #6319 maybe actually finally

Proposed Changes

Adds a new destroy lifecycle hook that is called at the end of dispose which can be used for cleanup (e.g. of any backing data models for the block).

Reason for Changes

Sometimes you need to clean data up!

Test Coverage

Adds tests to make sure dispose gets called at the proper time, and events fired from it are actually fired.

Documentation

N/A

Additional Information

Putting this up as a draft until we're sure we're definitely happy with the design.

@github-actions github-actions bot added the PR: fix Fixes a bug label Dec 2, 2022
@BeksOmega BeksOmega changed the title fix: add destroy lifecycle hook to blocks feat: add destroy lifecycle hook to blocks Dec 2, 2022
@BeksOmega BeksOmega added PR: feature Adds a feature and removed PR: fix Fixes a bug labels Dec 2, 2022
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Dec 2, 2022
@BeksOmega BeksOmega marked this pull request as ready for review December 5, 2022 17:46
@BeksOmega BeksOmega requested a review from a team as a code owner December 5, 2022 17:46
@BeksOmega BeksOmega requested a review from gonfunko December 5, 2022 17:46
core/block.ts Outdated
init?: (() => AnyDuringMigration)|null = undefined;

/** An optional method called during disposal. */
destroy?: (() => AnyDuringMigration)|null = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why AnyDuringMigration as the return type rather than void? It also seems like it might be nicer to make it non-optional and just initialize it to null rather than undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with void & removed the |null. I thought undefined made more sense as a default value, since this property /is/ optional, and its either defined or its not.

@BeksOmega BeksOmega merged commit 0ea319c into RaspberryPiFoundation:develop Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: feature Adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom handling when the block is disposed

2 participants