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

add beforeDestroy hook #9933

Merged
merged 5 commits into from Dec 5, 2021

Conversation

LeeLenaleee
Copy link
Collaborator

Resolves #9931

Copy link
Member

@kurkle kurkle left a comment

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
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
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
Copy link
Contributor

stockiNail commented Dec 1, 2021

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
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
Copy link
Member

kurkle commented Dec 1, 2021

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

@etimberg
Copy link
Member

etimberg commented Dec 1, 2021

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

@LeeLenaleee
Copy link
Collaborator Author

LeeLenaleee commented Dec 1, 2021

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
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.

Copy link
Member

@kurkle kurkle left a comment

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

src/core/core.controller.js Outdated Show resolved Hide resolved
@LeeLenaleee
Copy link
Collaborator Author

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

destroy_flowchart

@etimberg etimberg merged commit e7aec8c into chartjs:master Dec 5, 2021
@LeeLenaleee LeeLenaleee deleted the feature/beforeDestroyHook branch December 5, 2021 17:33
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