fix!: move destroy earlier in the lifecycle #7117
Conversation
There was a problem hiding this comment.
Can you update the PR description in the bottom section you added to be speaking directly to people who view this PR from the release notes? Coming from that mindset, it's not clear what my action item is here. e.g.
If you implement the destroy hook, then note that this hook will now be run before the connections and fields associated with this block are destroyed. This is unlikely to actually require changes in your project unless your destroy hook was explicitly relying on the old timing.
|
Also @mark-friedman can you check that this timing would work for you? |
I'll check. |
521e00a to
e291f3a
Compare
| this.inputList.length = 0; | ||
| this.getConnections_(true).forEach((c) => c.dispose()); | ||
| } finally { | ||
| eventUtils.enable(); |
There was a problem hiding this comment.
I don't think we need to disable events any longer since the delete event is fired by dispose and not disposeInternal, which is what we use to recurse.
Said another way, only the top level block fires a delete event, so we have no need to disable events for child blocks.
Here is the context for originally disabling events: c429949
|
I had to move things around to get tests to pass :/ Would you mind doing a re-review @maribethb ? No rush on this though. |
It looks like it would work. Thanks! |
|
Might I also suggest that you change the documentation for the |
The basics
npm run formatandnpm run lintThe details
Resolves
Fixes #6974
Proposed Changes
Moves the
destroyhook call up in the life cycle.Behavior Before Change
Before the destroy hook was called at the very end of disposal, so it couldn't depend on any of the other state of the block.
Behavior After Change
Now it is called after the block is disconnected from its parent, but before any of its elements or child blocks are disposed. So (e.g.) the
destroyhook could depend on the value of the block's fields.Reason for Changes
If you want to destroy external state that is associated with field values, you need access to the field values in the
destroyhook.Test Coverage
N/A
Documentation
Docs need to be updated for v10.
Additional Information
N/A
Breaking changes / updating / upgrading
If you implement the
destroyhook, then note that this hook will now be run before the connections and fields associated with this block are destroyed. This is unlikely to actually require changes in your project unless your destroy hook was explicitly relying on the old timing.