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

Observable#unbind: unbinding attribute which is not bound throws ambiguous error #4865

Closed
Reinmar opened this issue Mar 10, 2016 · 5 comments · Fixed by ckeditor/ckeditor5-utils#210
Labels
package:utils type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Mar 10, 2016

Moved from https://github.com/ckeditor/ckeditor5-core/issues/153

A.bind( 'x' ).to( B, 'a' );
A.unbind( 'totallyNotBoundToAnythingOrEvenNotExistingInA' );

Actual: TypeError: Cannot read property 'to' of undefined

Expected: A meaningful error.

@jodator
Copy link
Contributor

jodator commented Nov 27, 2017

I've just stumbled upon this. Still an issue... as in my feature I'm binding/unbinding stuff and there is no way to check if a binding already exists (AFAIK).

@oleq
Copy link
Member

oleq commented Nov 27, 2017

IMO unbinding a non-existent attribute should not throw an error. Since o.unbind() does not throw, neither o.unbind( 'nonexistent' ) should throw.

@oleq
Copy link
Member

oleq commented Nov 27, 2017

Still an issue... as in my feature I'm binding/unbinding stuff and there is no way to check if a binding already exists

Just to be clear, this sounds like another problem. If you meant double bindings like

A.bind( 'x' ).to( B, 'a' );
A.bind( 'x' ).to( B, 'a' ); // -> this one throws

this should be a separate issue. Perhaps we should not throw in such situation.

@jodator
Copy link
Contributor

jodator commented Nov 27, 2017

@oleq I'm describing problem from you first comment:

const a = new ObservableClass();
a.unbind( 'foo' ); // whoopsie :(

On the second I don't have an opinion... maybe it is OK to throw as we allow only one binding per attribute.

@jodator
Copy link
Contributor

jodator commented Nov 27, 2017

OK. I've created a rather simple fix for that issue only.

oleq referenced this issue in ckeditor/ckeditor5-utils Nov 27, 2017
Other: `ObservableMixin#unbind` should not throw if used for an attribute which is not bound. Closes #5.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-utils Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:utils labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:utils type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants