Skip to content
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

Add shiny:sessionInit event #1568

Merged
merged 6 commits into from Apr 5, 2017
Merged

Add shiny:sessionInit event #1568

merged 6 commits into from Apr 5, 2017

Conversation

@bborgesr
Copy link
Contributor

bborgesr commented Jan 31, 2017

This PR triggers a new JS event called shiny:sessioninitialized , which is fired at the end of the initialize method of the Session object (piggybacking on the config message, which is the last command of the initialize method). This allows us to listen for the shiny:sessioninitialized event when we want to get the value of things like Shiny.user.

@bborgesr bborgesr added the review label Jan 31, 2017
@@ -632,7 +632,12 @@ var ShinyApp = function() {

addMessageHandler('config', function(message) {
this.config = {workerId: message.workerId, sessionId: message.sessionId};
// != is on purpose (instead of !==) because it checks for both null and undefined
// (see here: http://stackoverflow.com/a/21273362/6174455)
/*eslint-disable */
if (message.user != null) exports.user = message.user;

This comment has been minimized.

Copy link
@wch

wch Jan 31, 2017

Collaborator

It's more idiomatic to use if (message.user) (to look for truthy values in general) or, if you want to be more precise, you could be explicit with (message.user !== undefined && message.user !== null). Either way is cleaner than adding the (necessary) comments explaining why it's written that way.

if (message.user != null) exports.user = message.user;
/*eslint-enable */
$(document).trigger('shiny:sessionInit');

This comment has been minimized.

Copy link
@wch

wch Jan 31, 2017

Collaborator

For consistency with other events, this should be all lower case. I think that shiny:sessioninitialized would be better, since it indicates that it happens after the initialization.

@bborgesr

This comment has been minimized.

Copy link
Contributor Author

bborgesr commented Jan 31, 2017

After a chat with @jcheng5, we're going to mull this over a little bit. For the particular use case that we wanted (fetching Shiny.user), we can use the following:

$(document).one('shiny:idle', function() {
    console.log(Shiny.user);
});

instead of the new way this PR provides, which is:

$(document).on('shiny:sessioninitialized', function() {
    console.log(Shiny.user);
});

The question is if there is still value in having a shiny:sessioninitialized event, or alternatively, if we could just move shiny:connected to fire only when shiny:sessioninitialized is currently firing (which would make it redundant and therefore unnecessary). For this last point, there are both questions of whether or not there is legitimate use of Shiny between the current shiny:connected and shiny:sessioninitialized, and back compatibility.

As of now, the order in which each event fires is:

connected -> sessioninitialized -> idle

@wch

This comment has been minimized.

Copy link
Collaborator

wch commented Mar 2, 2017

It would be a good idea to look at a search for shiny:connected to see if any of them need to shiny:connected to happen when it currently does.

@wch

This comment has been minimized.

Copy link
Collaborator

wch commented Mar 30, 2017

After we merge this, we should also update the documentation to tell people that this event is probably what they want to use, instead of shiny:connected.

@bborgesr

This comment has been minimized.

Copy link
Contributor Author

bborgesr commented Mar 30, 2017

Internal note: the plan is for us to merge this right after CRAN accepts 1.0.1

bborgesr added 2 commits Jan 31, 2017
@bborgesr bborgesr force-pushed the barbara/loaded branch from b35cd65 to d7a62d0 Apr 3, 2017
bborgesr and others added 4 commits Apr 3, 2017
@wch wch merged commit cf21e98 into master Apr 5, 2017
0 of 4 checks passed
0 of 4 checks passed
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@wch wch deleted the barbara/loaded branch Apr 5, 2017
@wch wch removed the review label Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.