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

Use up-to-date configuration reference in callbacks #125

Closed
wants to merge 1 commit into from

Conversation

mf-bruce
Copy link

Summary
Callbacks are registered during afterInit, however they were using a fixed reference to the dragData object using the chart instance object at time of initialization. That's fine only if the configuration options never change. However if, as was the case for me, the user is changing their callback definitions and updating the options object accordingly, the new callbacks would never be used by this plugin.

Test Plan

  • Instantiate a chart with one set of callbacks
  • Rerender the chart with a new set of callbacks
  • Plugin uses the most recent callbacks

**Summary**
Callbacks are registered during `afterInit`, however they were using a fixed reference to the `dragData` object using the chart instance object at time of initialization. That's fine *only* if the configuration options never change. However if, as was the case for me, the user is changing their callback definitions and updating the options object accordingly, the new callbacks would never be used by this plugin.

**Test Plan**
- Instantiate a chart with one set of callbacks
- Rerender the chart with a new set of callbacks
- Plugin uses the most recent callbacks
@mf-bruce
Copy link
Author

@chrispahm any thoughts on this?

@chrispahm
Copy link
Collaborator

Hey @mf-bruce,

I'm sorry for taking so long to answer, I don't work with Chart.js anymore so this work just comes on top of the other things... If you're interested in maintaining this repo (maybe @artus9033?) please write me a short mail (see profile)!

Note that there are still a lot of test cases missing (see #40), plus there are no unit test at all! This is the main reason why I started lagging behind all these PRs and issues, as all options need to be tested manually after each update. Maybe Github Copilot / ChatGPT could be of help writing these test but I wouldn't be too sure about it...

@artus9033 artus9033 added this to the Release of v2.3.0 milestone Jan 18, 2024
@artus9033 artus9033 added the bug label Jan 18, 2024
@artus9033 artus9033 modified the milestones: v3.0.0, v3.0.1 Aug 17, 2024
@artus9033
Copy link
Owner

This change is already present in the current v3.0.0, I implemented it in a very similar way during rewrite in TS in #146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants