destroy() method throws error upon second call. this.elements undefined

justsilencia commented 5 years ago

PLEASE BE AWARE Issue #411 does not solve this issue. My version already contains patch for #411 .

After initializing Croppie, then clicking cancel (calling .destroy()), all seems well and no errors in console. However, if you create a second Croppie instance after this, then call .destroy() again, you get:

TypeError: Cannot read property 'boundary' of undefined.

It also errors out if you try saving the crop rather than clicking cancel.

I’ve been pouring through the croppie file, and the error is perplexing. Error occurs on _destroy when it tries to access the self.elements object:

function _destroy() { var self = this; self.element.removeChild(self.elements.boundary); removeClass(self.element, 'croppie-container'); if (self.options.enableZoom) { self.element.removeChild(self.elements.zoomerWrap); } delete self.elements; }

For some reason self.elements is undefined, hence the type error. Although self.elements is deleted on destroy, it is re-created upon creation of new instance, so it shouldn’t evaluate to undefined. I verified the re-creation of self.elements in the console, which re-populates the object just fine:

    boundary = self.elements.boundary = document.createElement('div');
    viewport = self.elements.viewport = document.createElement('div');
    console.log(self.elements);

Expected Behavior

Call the destroy method then re-instantiate the croppie object multiple times without issue.

Actual Behavior

After the first call to destroy, this.elements loses its state somewhere along the line, leading to a failure of the croppie instance.

Steps to Reproduce the Problem

Instantiate the croppie object.
Call destroy method
Re-instantiate the croppie object (without refreshing the browser).
Call destroy again, or try cropping the image.

ghost commented 5 years ago

Hello! I was experiencing this issue and just wanted to share what fixed it. I was binding the 'destroy' call to a cancel button as an event listener. When I canceled, I was not removing that event listener correctly. So it was firing twice and causing the error.

justsilencia commented 5 years ago

@floifyed AAAhhhh, this is probably the answer! Total deficiency on my part for not catching that. For my solution, I ended up just toggling the visibility of a single croppie instance. So cancel doesn't destroy, it just hides the croppie view. It works great but I'll be switching this asap now that you pointed this out. Thanks for sharing!

ojack commented 4 years ago

To follow up with this, it seems the evenListener on the zoomerElement is not being destroyed when destroy() is called. I handled this by replacing the zoom slider element after calling destroy():

  var oldCrop = document.getElementById("crop-slider")
    var newCrop = oldCrop.cloneNode(true)
    oldCrop.parentNode.replaceChild(newCrop, oldCrop)

Euxiniar commented 4 years ago

To follow up with this, it seems the evenListener on the zoomerElement is not being destroyed when destroy() is called. I handled this by replacing the zoom slider element after calling destroy():
  var oldCrop = document.getElementById("crop-slider")
    var newCrop = oldCrop.cloneNode(true)
    oldCrop.parentNode.replaceChild(newCrop, oldCrop)

Where did you set the 'crop-slider' id? If I try your code, the program doesn't find any element with this id.

ojack commented 4 years ago

Oh oops, I realized that I had modified my local version of Croppie in order to create a custom zoomer element, so this code is irrelevant.....sorry!

Euxiniar commented 4 years ago

Oh oops, I realized that I had modified my local version of Croppie in order to create a custom zoomer element, so this code is irrelevant.....sorry!

Ok no problem. I did something a bit dirtier but at least it works haha

guilhermebentomarques commented 3 years ago

Any news on that?

Foliotek / Croppie