daattali / shinycssloaders

⌛ Add loading animations to a Shiny output while it's recalculating
https://daattali.com/shiny/shinycssloaders-demo/
Other
395 stars 45 forks source link

Undefined event target ids cause javascript errors #33

Closed hillalex closed 4 years ago

hillalex commented 5 years ago

Firstly, thanks for this plugin, it's super useful and the version that's currently on cran works great! But the latest GitHub source has the following issue:

Event handlers are currently attached to the document like so:

$(document).on('shiny:value shiny:error', function(event) {
  output_states[event.target.id] = 1;
  update_spinner(event.target.id);
});
}());

This code works fine as long as the event target is a div with an id (as all shiny outputs will be) but in some instances these events fire with the document as the target, which has no id, while update_spinner(id) requires id to not be undefined, as of this PR being merged: https://github.com/andrewsali/shinycssloaders/pull/27

This can cause a javascript error which is fatal to the app's proper functioning.

Should say, this does seem like a bug in shiny as it directly contradicts the shiny js events documentation which states that the target of these events should be an output object (https://shiny.rstudio.com/articles/js-events.html), but I'm seeing these events fire with the document as the target on multiple shiny apps. Given this behaviour, this plugin becomes unusable.

daattali commented 4 years ago

A lot of work went recently into fixing bugs and error messages and adding new features. Since you did not include a code sample, I assume this problem was fixed (there are other issues that were opened that results in the same error message that are now fixed).

If you still have problems, feel free to open an issue with a code sample.