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

Fix sporadic dates (#3664) #3665

Merged
merged 19 commits into from
Sep 30, 2024
Merged

Fix sporadic dates (#3664) #3665

merged 19 commits into from
Sep 30, 2024

Conversation

dvg-p4
Copy link
Contributor

@dvg-p4 dvg-p4 commented Jul 7, 2022

Closes #3664.

This pull request removes the handlers for keyup and input from dateInput and dateRangeInput. This prevents spurious updates while typing, but still sends the parser's best interpretation of the field when enter is pressed, focus is lost (i.e. the user clicks out), or a date selection is made in the GUI (due to the remaining changeDate and change handlers).

A value of null (for dateInput) or NA (for dateRangeInput) is still sent immediately when the field is cleared, but this can be more easily checked for and ignored on the server side. I suspect that bootstrap-datepicker is interpreting the empty string as a special value and firing off the changeDate event, and stopping that would probably be much more intensive than removing a few lines of code.

Not sure if y'all will want to shove this into 1.7.2, leave it for the next release, or if there's some crazy use case I can't imagine where you'd want "2001-01-0" to be immediately interpreted as the last day of 1999.

Edit: Merged in 1.7.2-1.7.4 changes, no conflicts except for minified JS as usual.

Edit 2: Merged in all changes up to the current development version 1.9.1.9000, again no conflicts except for minified JS.

This prevents spurious updates while typing, but still sends when enter is pressed, focus is lost, or the GUI is clicked (due to the remaining `changeDate` and `change` handlers).
Conflicts:
	NEWS.md
	inst/www/shared/shiny.js
	inst/www/shared/shiny.js.map
	inst/www/shared/shiny.min.js
	inst/www/shared/shiny.min.js.map
@CLAassistant
Copy link

CLAassistant commented Nov 8, 2023

CLA assistant check
All committers have signed the CLA.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Sep 24, 2024

Looks like everyone else's PRs are also getting the same error on ubuntu-20.04 (4.1.3), so that's almost certainly not my fault.

The yarn stuff might be, but I just ran the standard commands -- yarn install; yarn build, with the latest yarn and npm installed; so I'm not sure what's going wrong there. It looks like the checksum for selectize is changing, despite being pinned to a specific commit?

Hmm, looks like similar has happened before:
image
(Note the specifications for this package are all "last year" but the checksum is "10 months ago")

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Sep 24, 2024

And I'm also not sure why license/cla Expected — Waiting for status to be reported is hanging--I signed the CLA 2 years ago. Actually maybe that's the problem if you've updated it since, but the bot doesn't realize it and isn't giving me the right link to click.

image

@cpsievert any idea how to fix this?

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Sep 24, 2024

Aha, the yarn issue may be due to yarnpkg/berry#2399 / yarnpkg/berry#2774 (comment), since I'm building on a Mac and the github checks are on Ubuntu.

...but, I get the same checksum when building on a Rocky 8 box, so maybe that's not it. Unless MacOS and Rocky8 match while Ubuntu is different, but that'd be strange.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Sep 24, 2024

Downgrading my node/npm to match the versions listed here does not change my checksum for this package.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Sep 24, 2024

Hah, looks like that got the test to pass...might be worth looking into what exactly's wrong with the checksum, but I suspect it's a yarn bug rather than a shiny bug.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Sep 24, 2024

Remaining test failure is due to #4133

Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Thanks @dvg-p4 for the fix and for patiently keeping this PR up to date. We agree that the current behavior of sending eager updates is problematic and that your solution of not reporting updates on keydown is a good choice.

I have a few minor edits to comments and the news entry. I've also manually verified that we do have your CLA on file; I wasn't able to figure out what is happening with the cla-assistant but we don't need the check.

Thanks again!

srcts/src/bindings/input/date.ts Outdated Show resolved Hide resolved
srcts/src/bindings/input/daterange.ts Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@gadenbuie gadenbuie merged commit abf7138 into rstudio:main Sep 30, 2024
12 checks passed
@dvg-p4 dvg-p4 deleted the fix-sporadic-dates branch September 30, 2024 21:27
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.

dateInput and dateRangeInput send spurious values to server mid-typing
3 participants