Multiple event handlers on single loadingButton

[bartekch]

I bumped into this when investigating #69. When creating loadingButton with already existing inputId, it is correctly removed from buttons array, but it looks like click event handlers are accumulated. I added some logs to check this, https://github.com/bartekch/shinyFeedback/commit/d7878b8b45959bf65ce4de8c05df5a6f86ad87b7.

remotes::install_github("bartekch/shinyFeedback", "d7878b8b45959bf65ce4de8c05df5a6f86ad87b7")

library("shiny")

shinyApp(
  ui = basicPage(
    shinyFeedback::useShinyFeedback(),
    actionButton("rerender", "Rerender button"),
    uiOutput("button"),
    verbatimTextOutput("b_val"),
    verbatimTextOutput("val")
  ),
  server = function(input, output, session) {
    output$button <- renderUI({
      input$rerender
      shinyFeedback::loadingButton("b", "loadingbutton")
    })

    output$b_val <- renderPrint(input$b)
    val <- eventReactive(input$b, {
      shinyFeedback::resetLoadingButton("b")
      rnorm(1)
    })
    output$val <- renderPrint(val())
  }
)

After a few clicks and renders console output looks like this.

To be honest I didn't find any harmful effect on the actual Shiny application, however it still doesn't seem to be correct.

One simple way to fix this is to add $(document).off('click', "#" + inputId); before adding new listener. However this would remove all click events handlers, which may be undesired (or maybe this is not a problem?). To fix this we could pass handler to buttons array, so later we could remove an exact handler when an existing id is encountered.

  click_button = function() {
    // increment the button value by 1.  This is consistent with how `shiny::actionButton`
    // value works.
    console.log("Click button " + id + " with value ", btn_value);

    if (btn_value === null) {
      btn_value = 1;
    } else {
      btn_value = btn_value + 1;
    }
    Shiny.setInputValue(inputId, btn_value);

    var loading_button = $(this);

    loading_button.attr('disabled', true);
    loading_button.html('<i class="fas fa-' + options.loadingSpinner + ' fa-spin"></i> ' + options.loadingLabel);
    loading_button.attr('style', options.loadingStyle);
    loading_button.removeClass(options["class"]);
    loading_button.addClass(options.loadingClass);
  };
  
  $(function() {
    var allDOMLoadingButtons = $(document).find(".sf-loading-button");
    var loadingIds = [];
    for(var obj of allDOMLoadingButtons) {
      loadingIds.push(obj.id);
    }
    
    // if element in this.buttons represents a loadingButton that is no longer in the DOM
    // then remove it from this.buttons.  Also remove remove it if the button being added already
    // exists in this.buttons
    self.buttons = self.buttons.filter(obj => {
      if (obj.inputId == inputId) {
        $(document).off('click', "#" + inputId, obj.click_handler);
      };
      return loadingIds.includes("sf-loading-button-" + obj.inputId) && (obj.inputId !== inputId);
    });  
  
    self.buttons.push({inputId: inputId, options: options, click_handler: click_button}); 
  });
  
  // Disable button & change text
  $(document).on('click', "#" + inputId, click_button);

I'm not JS expert, so probably it could be done better. Anyway, I've made a draft pull request with my suggestions: https://github.com/merlinoa/shinyFeedback/pull/70. There are still some console.log calls there, I'd be happy to clean it after your suggestions.