feat!: allow blocks to receive their own delete events #6337
Conversation
There was a problem hiding this comment.
Approved subject to minor nits.
I'm not sure what's up with you having to move the test files in order for them to run. Can you be more specific about how they were failing. Were they just being ignored?
In the mean time: would it be better to rename the moved test files to tests/mocha/blocks_*_test.js (or …/libraryBlocks_*_test.js) to make it more clear what they are testing? (E.g. Blockly core doesn't really have any "lists" that need testing…)
| @@ -980,7 +980,7 @@ const PROCEDURE_CALL_COMMON = { | |||
| Xml.domToWorkspace(xml, this.workspace); | |||
| Events.setGroup(false); | |||
| } | |||
| } else if (event.type === Events.BLOCK_DELETE) { | |||
| } else if (event.type === Events.BLOCK_DELETE && event.blockId != this.id) { | |||
There was a problem hiding this comment.
Strictly speaking this is a breaking change and should probably be mentioned in the PR description, even if it's very unlikely to ever cause anyone an issue.
There was a problem hiding this comment.
I don't think this is a breaking change. Before blocks did not receive their own events, so this case would have never been hit. Now I've added the blockId check so it continues to not be hit.
There was a problem hiding this comment.
onchange is not private, so it could be called by code outside Blockly, which could pass in an arbitrary event.
I agree this is unlikely in practice, which is why I said "strictly speaking". It's probably not useful to bump a major version number or to mention in the release notes, but it probably is worth making sure that I am not the only one who has considered this scenario, and hence the comment.
They were just being ignored :/
I would prefer to fix whatever the issue is. I think they were actually originally prefixed with |
Having noticed a bunch of resource not found errors in the console when running mocha tests in |
BeksOmega
added
PR: feature
|
I had a quick look at the final version of this PR, and it LGTM, but the post-review commit history is so convoluted that I would not say I was exactly happy to see it submitted based on my earlier approval. A judicious force-push to consolidate all the post-review changes into one or two easy-to-vet commits would definitely have made this less worrying. |
BeksOmega
removed
the
breaking change
The basics
npm run formatandnpm run lintThe details
Resolves
#6319
Proposed Changes
Allows blocks to receive their own dispose events.
Behavior Before Change
Change listeners would be removed, and then dispose events would be fired (so blocks would not receive their own dispose events).
Behavior After Change
Dispose events are fired and then change listeners are removed (so blocks do receive their own dispose events).
Reason for Changes
Sometimes blocks need to clean up state when they are disposed, this allows that to happen.
Test Coverage
Documentation
It would be nice if we had some documentation about block lifecycle.
Additional Information
@cpcallen I had to move the block tests back into the main mocha folder to get them to run. Would love to know what I need to do to get them to run from the
mocha/blocksdirectory.Breaking Changes
This is a breaking change, because it could cause event listeners to be triggered when they were not previously. In particular, if you are using a deny list instead of an allow list. The recommended solution is to use an allow list in your change handler instead of a deny list.