Skip to content

#45: Configurator error on destroy #227

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

Merged
merged 3 commits into from
Dec 22, 2020

Conversation

BeeMargarida
Copy link
Contributor

- -
Issue ripe-tech/ripe-sdk-components-vue#45
Dependencies --
Decisions Due to the async nature of the configurator and image execution, it is possible to initiate the destruction of a component that uses the configurator and images while they are updating their state. This would result in the DOM element not existing anymore and the components would try to access them, resulting in errors. The solution was returning the functions if the element was not defined.
Animated GIF Showing no errors when initiating and destroying components rapidly: destroy_configurator

@BeeMargarida BeeMargarida added the bug Something isn't working label Dec 14, 2020
@BeeMargarida BeeMargarida requested a review from gcandal December 14, 2020 16:13
@gcandal
Copy link
Contributor

gcandal commented Dec 14, 2020

Can you post the sequence of SDK method calls that leads to this error? The reason I'm asking is that:

  • If the error is raised in sync methods, then there is nothing to fix because you aren't supposed to call methods on a configurator after you've called unbind on it
  • If the error is raised because you've called an async method and only unbinded the configurator afterwards, then the async method you called should have been cancelled

@gcandal gcandal assigned BeeMargarida and unassigned gcandal Dec 14, 2020
@BeeMargarida
Copy link
Contributor Author

Can you post the sequence of SDK method calls that leads to this error? The reason I'm asking is that:

* If the error is raised in sync methods, then there is nothing to fix because you aren't supposed to call methods on a configurator after you've called unbind on it

* If the error is raised because you've called an async method and only unbinded the configurator afterwards, then the async method you called should have been cancelled

I believe this in the storybook is the extreme, since it is not supposed to destroy and initiate components this way in normal applications. The problems in the configurator consist of several methods, for example the _preload function and its methods like _mark, which run after a delay. The configurator may have been destroyed but this method will later try to access this element.

element.classList.remove("preloading");

It would result in an error like:

Uncaught (in promise) TypeError: this.element is null
    mark configurator-prc.js:1305
    render configurator-prc.js:1351
    promise callback*render configurator-prc.js:1351
    render configurator-prc.js:1353
    render configurator-prc.js:1353
    result configurator-prc.js:1369
    setTimeout handler*./node_modules/ripe-sdk/src/js/visual/configurator-prc.js/ripe.ConfiguratorPrc.prototype._preload/result< configurator-prc.js:1367
    _preload configurator-prc.js:1276
    update configurator-prc.js:247
    _update ripe.js:1256
    update ripe.js:1315
    setPart ripe.js:716
    _callee2$ ripe-pickers.vue:185
    Babel 8
    VueJS 3
configurator-prc.js:1305

Another example is the _updateConfig, which is called in init but is async and does not have an await:

this._updateConfig();

It also happened once in the image update method (this is the one that is stranger), which in fact already had the check later in the method

if (!this.element) return;

@gcandal
Copy link
Contributor

gcandal commented Dec 15, 2020

@BeeMargarida please check:

My whole question about this: is this reproducible if you use the SDK correctly by only destroying the HTML element after unbind has completed?

@BeeMargarida
Copy link
Contributor Author

BeeMargarida commented Dec 15, 2020

@BeeMargarida please check:

* if using https://vuejs.org/v2/api/#beforeDestroy to unbind the configurator prevents the problem from happening

* if this is unit-testable

My whole question about this: is this reproducible if you use the SDK correctly by only destroying the HTML element after unbind has completed?

@gcandal So, I found a simpler and more "scoped" solution. There are 3 major problems:

  • The destroyed (same for beforeDestroy) can happen before the bindConfigurator call, where the destroyed call ends and the configurator DOM element does not exist, but it will implement the bindConfigurator with the configurator $ref as undefined.
    The solution I propose is this, both in configurator and image: ripe-sdk bind calls return if the element is not defined and, verify in ripe-configurator and ripe-image if the result from the bind operation is defined. Or, only by changing the sdk-components, verify if the $refs exists in the components before calling the bindConfigurator and bindImage methods.

Image showcasing the destroyed and created calls and the error:
image
image


  • The render and the _drawMask methods in configurator-prc run after a delay. Both these functions call others that access the element, which may already have been destroyed.
    One solution would be to verify if the element is undefined in these methods and return. Other would be to save this as promises and cancel them in the cancel method, similar to the loadedCallback in the image here and here. I'm not sure this last solution is possible. Nevertheless, I think it is necessary to modify ripe-sdk to solve this problem.

image


  • The onload call for _drawMask that happens in configurator-prc. This one is very similar to the one above, and the solutions are the same.

image(1)


Almost forgot, this whole problem might be reproducible if the is some conditional rendering in the utilization of these components, where a toggle in a button might change from one to another. It might be a very specific situation though, not sure if it would be common.


Regarding unit testing, the errors only appear in the console but do not hinder the utilization of the components or the initialization of other components. I think it would be possible to destroy the component and see if the configurator method deinit was called or something similar.


Regarding awaiting for the async mounted, I don't think vue supports it. To change only in the sdk-components I think it would be best to check if $refs.configurator is defined before bindConfigurator.

@gcandal gcandal assigned joamag and unassigned BeeMargarida Dec 22, 2020
@joamag joamag merged commit f2a66ef into master Dec 22, 2020
@joamag joamag deleted the ms/45-configurator-error-on-destroy branch December 22, 2020 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants