Skip to content

add beforeDestroy hook#9933

Merged
etimberg merged 5 commits into
chartjs:masterfrom
LeeLenaleee:feature/beforeDestroyHook
Dec 5, 2021
Merged

add beforeDestroy hook#9933
etimberg merged 5 commits into
chartjs:masterfrom
LeeLenaleee:feature/beforeDestroyHook

Conversation

@LeeLenaleee

Copy link
Copy Markdown
Collaborator

Resolves #9931

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should add to documentation.

@LeeLenaleee

Copy link
Copy Markdown
Collaborator Author

Thats what I thought but when I looked at the plugins hook section it only showed render diagrams for the inialization, update, render and event prosses.

Did I miss a place in the docs where all the hooks get described because I couldnt find it, also not with the search bar?

Or do you suggest adding an extra diagram for destroying the chart on that plugin section?

@stockiNail

Copy link
Copy Markdown
Contributor

@LeeLenaleee let me ask you if makes sense to you to change also the destroy hook in afterDestroy (of course moving the notification after delete of instance). Maybe in this way it could be consistent with the other plugin hooks.
But I know, this would be a breaking change.

@stockiNail

stockiNail commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

Did I miss a place in the docs where all the hooks get described because I couldnt find it, also not with the search bar?

docs/developers

docs/developers/plugin_flowcharts.drawio

@LeeLenaleee

Copy link
Copy Markdown
Collaborator Author

@LeeLenaleee let me ask you if makes sense to you to change also the destroy hook in afterDestroy (of course moving the notification after delete of instance). Maybe in this way it could be consistent with the other plugin hooks.
But I know, this would be a breaking change.

Yeah this does indeed make sense because it happens after the destroy and would be more in line with the current naming of hooks. But like you said this will be a breaking change so best thing to do I think is make a new issue for this to rename it and target v4 with it.

docs/developers/plugin_flowcharts.drawio

Thanks, means I didnt miss a spot. The uninstall prosess was not being described but I will make a flow for that one and add it 👍

@kurkle

kurkle commented Dec 1, 2021

Copy link
Copy Markdown
Member

We could add the afterDestroy already and leave the destroy hook to be removed in V4.
@etimberg thoughts?

@etimberg

etimberg commented Dec 1, 2021

Copy link
Copy Markdown
Member

That works for me. We can just deprecate the destroy hook but leave it in until v4

@LeeLenaleee

LeeLenaleee commented Dec 1, 2021

Copy link
Copy Markdown
Collaborator Author

I saw that the destroy hook was called before the deletion of the chart instance from the instances object. Shouldnt the destroy and afterDestroy hooks be called after this deletion happend at the very end of the destroy function?

FlowChart:
image

etimberg
etimberg previously approved these changes Dec 1, 2021
@stockiNail

Copy link
Copy Markdown
Contributor

Shouldnt the destroy and afterDestroy hooks be called after this deletion happend at the very end of the destroy function?

As user, I'd expect after the deletion from the instances. But this is just my personal opinion.

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Notify stop two times in the flow chart

Comment thread src/core/core.controller.js Outdated
@LeeLenaleee

Copy link
Copy Markdown
Collaborator Author

Seems I coppied the flow diagram in wrong state, current code had correct version:

destroy_flowchart

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.

Custom plugin does not have beforeDestroy call

4 participants