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

Trigger shiny:value even if same data is received #1999

Merged
merged 2 commits into from Apr 18, 2018

Conversation

andrewsali
Copy link
Contributor

This PR fixes #1978 . Without this PR, if the client receives the same data from the server, no shiny:value is triggered. On the other hand the client sees that the value was invalidated previously ( shiny:outputinvalidated is triggered), but unfortunately doesn't know when the old value is confirmed to be still valid.

The role of this PR is to bring back the symmetry: if shiny:outputinvalidated is triggered on an element, then eventually a shiny:value (or shiny:error) should also be triggered.

$(binding ? binding.el : document).trigger(evt);
return undefined;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is incorrect; the event shouldn't be triggered until after this.$values[name] = value; and delete this.$errors[name];. If for no other reason, errors in the trigger shouldn't stop those side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two cases, as in the version without the PR: a) client receives the same value as before, b) client receives a new value.

In case a) there is no need to assign the value (this should be more efficient as the DOM doesn't need to be updated) and there should be no errors to delete. This is the same behaviour as before, the only change here is that the event is now triggered (the event parameters need to be set up before).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this looks good. I don't know what I was thinking. 😅

$(binding ? binding.el : document).trigger(evt);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triggered a second time? (Maybe I'm missing something)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is case b) when the value actually needs to be updated. The event needs to be triggered in this case as well.

@jcheng5
Copy link
Member

jcheng5 commented Apr 12, 2018

Thanks for your patience.

One last thing: to accept contributions to RStudio packages, we need a signed the individual or corporate contributor agreement as appropriate. You can send the signed copy to jj@rstudio.com. (Once you've signed, you're good across all RStudio repos)

Thanks for the contribution!

@andrewsali
Copy link
Contributor Author

Thanks @jcheng5 , I have sent now the contributor agreement.

@andrewsali
Copy link
Contributor Author

Hi @jcheng5, is there anything holding back this PR?

@jcheng5
Copy link
Member

jcheng5 commented Apr 18, 2018

@andrewsali No, it's going in this week, probably today

@andrewsali
Copy link
Contributor Author

OK, thanks, that's much appreciated

@jcheng5 jcheng5 merged commit 101d9aa into rstudio:master Apr 18, 2018
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.

shiny:value not triggered if same table data received
2 participants