Skip to content

fix cache#5

Open
lz100 wants to merge 6 commits intodaattali:masterfrom
lz100:master
Open

fix cache#5
lz100 wants to merge 6 commits intodaattali:masterfrom
lz100:master

Conversation

@lz100
Copy link
Copy Markdown

@lz100 lz100 commented Nov 27, 2020

No description provided.

@daattali
Copy link
Copy Markdown
Owner

Thanks for the detailed issue report and taking the time to submit a PR! I really appreciate it. I haven't yet looked at the PR, I'll have to take the time to look into the code more closely, but just from reading the steps you describe in the issue it sounds good.

  • I understand you're adding a timer, can you confirm that this timer will not be always active and will always get deleted soon after the app is started? I want to ensure there isn't any performance degradation by having an infinitely running timer.

  • Has this solution been tested in multiple scenarios, such as: running locally in RStudio and killing the app vs running in a shiny server and stopping the server, made sure the error is seen whether the server dies or whether an error in the app is encountered

  • Regarding the last point about the demo app: It's true that the demo app is doing something funky and I'm sure you understand why. Even though this isn't the expected way to use the package, I do like the fact that {shinydisconnect} still works even in this odd case of dynamically inserting and immediately killing the app. Does this also mean that any app that gets errored out within 3 seconds will also have a similar issue?

@lz100
Copy link
Copy Markdown
Author

lz100 commented Nov 30, 2020

  • I use clearInterval to stop the timer. I tested and see it indeed stopped. You can try to add a console.log(num) between line 29 and 30 to see if it still runs.
  • I tested all your examples locally and deploy your demo to my account and they all worked for me.
  • Not all apps, only apps insert the script at the last second. To have my js function to run, it has to meet 2 requirements: 1. script inserted; 2. document ready. Usually when people use this package, script inserted into shiny UI and then the server runs, 1 -> 2. Your demo's server runs first and then script inserted when app stops, 2 -> 1. So scripts meet the requirements after the server ends and then start to run. So, 'shiny:disconnected' will first trigger error message, and then the timer cannot find server and report no connection.

Comment thread R/utils.R Outdated
Comment thread inst/examples/demo/ui.R Outdated
Comment thread inst/src/js/shinydisconnect.js Outdated
@daattali
Copy link
Copy Markdown
Owner

daattali commented Dec 2, 2020

Thanks, could you also add an entry to the NEWS file please

@lz100
Copy link
Copy Markdown
Author

lz100 commented Dec 3, 2020

Done. I check all examples again, no problem, but didn't deploy to check remotely this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants