Refactor shinyAce

[detule]

Hello!

First of all let me apologize for the breadth of the single commit in this PR - if you find value in it, I am happy to work with you on breaking it up into more maneagable pieces.

My use case is a dashboard that makes extensive use of modules, and with multiple editors embedded in the page. I was struggling with:

• Some of the inputs used for communicating between the JS environment and the [R] server not being module friendly.
• Some of the inputs used for communicating between the JS environment and the [R] server not being friendly to multiple editors being embedded in the page.
• Most of all, the congestion on the page caused by multiple editors inserting (the same) JS code snippets.

To alleviate this, I (below taken from the commit message):

• Rather than building a script and inserting it in the HTML body every time an editor is created, we move the element creation logic in the "initialize" step of the binding.
• [R] functions (aceEditor, updateAceEditor) create a JSON payload only that is attached to the appropriate element.
• Shared Javascript code-path for widget initialization, and widget update (shinyAce.js: updateEditor).
• Make JS <-> server.R communication fully module friendly (selectionId, cursorId, hotkeys). These are all now reported with a prefix "inputId_" (similar to shinyAce_hint). This is a breaking change.

Thanks!

[vnijs]

I am open to this although I am a bit concerned about breaking changes. Would you be willing to help out with (new) issues that might pop-up due to the proposed changes?

I mainly use shinyAce in my package https://cran.r-project.org/web/packages/radiant/index.html. However, when trying to execute code in Report > Rmd I'm getting the following error. FYI I don't use modules in this app. Any suggestions?

cc-ing @saurfang

[detule]

Thanks for giving this a look, and I can appreciate the hesitation to break existing code. Let me know if you have any ideas on how it can be avoided.

Happy to help out with problems as they arise in the future, time permitting.

Apologies for the $el derp; oversight on my part.

[vnijs]

Thanks @detule. The JS error is now gone, however, auto-complete and line selection no longer work. Can you give some more information about what changes are likely needed in existing code?

[detule]

As far as line selection, I suspect it's a matter of observing input$[editorname]_selection rather than input$selection.

For auto-complete I am not entirely sure. Perhaps we can narrow it down - do examples 06 and 07 behave as expected for you?

[vnijs]

@detule I double checked but the id I have been using for the selection was already in the format you suggested (see link below).

https://github.com/radiant-rstats/radiant.data/blob/master/inst/app/tools/app/report_rmd.R#L336-L355

The autocomplete examples in shinyAce do work but, for some, break autocompletion are now broken in radiant.data (Report > Rmd and Report > R)

[detule]

Thanks again for your patience here.

• Because the inputId is automatically prepended - this I believe is necessary in order for name-spacing in the presence of modules to be respected - in the example you linked I suspect selection events would be communicated via input$rmd_edit_rmd_edit_selection
• I am not sure what's the cuprit wrt autoComplete. Happy to try and debug a smaller example if you have one.

[vnijs]

@detule I figured out what was going on in my app. selectionId and the hotkey name will be pre-pended with the aceEditor id now. Although this does make using shinyAce with modules easier it also is a breaking change to an established package.

Before merging I'd like to check with two recent contributors (cc-ed) to get their input and thoughts.

cc @saurfang @GregorDeCillia

[GregorDeCillia]

@vnijs: Thanks for cc-ing. I am running into problems with aceAutocomplete inside shiny modules. For the following app, autocompletion works with

remotes::install_github("trestletech/shinyAce", ref = "690491e")

but not with

remotes::install_github("detule/shinyAce", ref = "7ab2d2c")

The refs correspond to the current versions of

• the master branch of trestletech/shinyAce and
• the pr-refactor branch from detule/shinyAce

App

library(shiny)

editorModuleUI <- function(id) {
  ns <- NS(id)
  shinyAce::aceEditor(
    ns("editor"), 
    mode = "r", 
    autoComplete = "live"
  )
}

editorModuleServer <- function(input, output, session) {
  shinyAce::aceAutocomplete("editor")
}

shinyApp(
  ui = fluidPage(
    editorModuleUI("myEditor")
  ),
  server = function(input, output, session) {
    callModule(editorModuleServer, "myEditor")
  }
)

Shiny version (newest CRAN)

sessioninfo::package_info(pkgs = "shiny", dependencies = FALSE)
#> package * version date       lib source        
#> shiny   * 1.3.2   2019-04-22 [2] CRAN (R 3.6.0)

---

The version above, where aceAutocomplete is called without the ns function, is the way this is supposed to work. See the shiny modules docs, which discourage the use of ns in the server part of modules.

As for the module server functions, just ensure that the call to callModule for the inner module happens inside the outer module’s server function. There’s generally no need to use ns().

See also #49 for the steps that were undertaken to make aceAutocomplete work with modules in the first place

[detule]

Thanks @GregorDeCillia - appreciate the minimal example.
If you don't mind and have the time - do you mind trying the tip of the pr-refactor branch again?

Nice work with making the rlang autoCompletion work with modules in #49. Hopefully in this PR we are not walking back any of that effort, but further extending the shinyAce + modules functionality.

[GregorDeCillia]

You are very welcome @detule. I just updated to the top of pr-refactor and autocompletion is working again. I tested with rlang 0.3.4 and rlang 0.4.0 (came out today).

[vnijs]

Thanks @GregorDeCillia. I will assume this means you are ok with the refactor by @detule.

@detule Can you add a few lines at the top of the README.md file so indicate that (1) shinyAce 0.4.0 has breaking changes and (2) what those changes are and how to address them in existing code?

Once we have that I will add @detule as a contributor and submit to CRAN unless @saurfang sees any remaining issues

[GregorDeCillia]

@vnijs shouldn't the breaking changes be noted in NEWS.md rather than README.md?

I'll do some quick checks tomorrow on a bigger project of mine that uses submodules and automatically generated IDs and report back. But I'm pretty sure there will be no issues.

[vnijs]

@GregorDeCillia Yes. We will also list the breaking changes in NEWS.md. However, README.md is a form of (limited) documentation. It should include the new examples as well and all examples should be updated to reflect the breaking changes (e.g., 01-basic should use selectionId as it not longer works).

Each listed example could then mention something like: "Note: As of shinyAce version 0.4.0 ..."

I also think we should also use something like the following to start the examples so the user sees the apps in display mode.

shiny::runApp(system.file("examples/06-autocomplete", package="shinyAce"), display.mode = "showcase");

[vnijs]

I'd like to add a placeholder argument to shinyAce based on the link below. Something like the below should work I believe but would need to be integrated with your other changes. I expect placeholder would be NULL by default but the user can provide a string to use as input.

https://stackoverflow.com/questions/26695708/how-can-i-add-placeholder-text-when-the-editor-is-empty

    if (data.hasOwnProperty('placeholder')) {
      function update() {
        var shouldShow = !editor.session.getValue().length;
        var node = editor.renderer.emptyMessageNode;
        if (!shouldShow && node) {
          editor.renderer.scroller.removeChild(editor.renderer.emptyMessageNode);
          editor.renderer.emptyMessageNode = null;
        } else if (shouldShow && !node) {
          node = editor.renderer.emptyMessageNode = document.createElement("div");
          node.textContent = data.placeholder;
          node.className = "ace_invisible ace_emptyMessage";
          node.style.padding = "0 5px";
          editor.renderer.scroller.appendChild(node);
        }
      }
      editor.on("input", update);
      setTimeout(update, 100);
    }

[GregorDeCillia]

I just tested the newest version of detule/shinyAce with a bigger project of mine and as expected, everything I use from shinyAce (hints, theme changes) still works.

[vnijs]

I'm going to merge this PR and do some work on documentation. Thanks @detule!

[vnijs]

@detule I just pushed some work updating the examples and the README.md. However, I noticed that since the latest update you made, autocomplete in example 6 no longer seems to work. Can you take a look?

[vnijs]

@detule FYI I found the problem and pushed a fix