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

Provide an empty object when the target of the resolver is undefined #10655

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

stockiNail
Copy link
Contributor

Fix #10654

@etimberg etimberg added this to the Version 4.0 milestone Sep 10, 2022
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Is there any way to test this?

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.

I'd try to make sure _getTarget returns an objet (and that same object is returned on subsequent call)

If my thinking is right, this could happen when user is setting some value to config and if the object is not stored, the value will not be stored either.

@stockiNail
Copy link
Contributor Author

@kurkle this is happening in annotation plugin when the user set a label to null in the configuration.
I agree that we should fix getTarget method.
To be homest I was lost reading the proxy code… I will be more than happy to see your solution so I can learn more how proxies are working

@stockiNail
Copy link
Contributor Author

stockiNail commented Sep 14, 2022

@kurkle I spent time to have a look to the code. My knowledge of that code is quite low.

_getTarget can have 2 different functions:

  1. () => scope[0]
  2. () => subGetTarget(resolver, prop, value)

The 1, when a "main" resolver is created, the 2 when a sub resolver is created.

The 1 sounds working correctly, returning the first scope
The 2 is a recursive call which returns the target, setting is not exist.

The case of the bug is when a target (label) for a property (callout) is undefined.

Following your suggestion, subGetTarget must return always an object, therefore the check must be implemented at "return" statement of the function.

That's my analysis but maybe I have misunderstood and I'm wrong. I have changed the PR accordingly on what I wrote above.
Let me know if this is what you thought.

EDIT: I don't think we need any check to do in case 1 because I don't think we can create a resolver without any scope.

@stockiNail
Copy link
Contributor Author

Is there any way to test this?

@etimberg apologize I didn't read your comment. I'm trying to do it without annotation plugin. I'll come soon with a feedback

@stockiNail
Copy link
Contributor Author

Is there any way to test this?

@etimberg I have added test case.

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.

Thanks @stockiNail
I haven't had the time to go through this, but the added test looks solid.

@etimberg etimberg merged commit d4e106c into chartjs:master Sep 15, 2022
@stockiNail stockiNail deleted the fixSubGetTarget branch September 15, 2022 16:15
@stockiNail
Copy link
Contributor Author

I haven't had the time to go through this, but the added test looks solid.

@kurkle no problem at all! I was pleasure and mainly helpful because I was able to understand better the proxies implementation. ;)

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

Successfully merging this pull request may close these issues.

Error from undefined label: "Cannot use 'in' operator to search for 'callout' in undefined"
4 participants