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 bug where unmounting and remounting the school autocomplete would… #19966

Merged
merged 1 commit into from Jan 12, 2018

Conversation

aoby
Copy link
Contributor

@aoby aoby commented Jan 12, 2018

… cause it to show blank even when it had a value

This is a bit of a weird case, which was revealed in the teacher application. I'm not sure what VirtualizedSelect is doing under the hood with its caching logic, but it doesn't work in our case. Disabling the cache fixed it.

Repro: when the school autocomplete was unmounted and then remounted, it would always show blank whether or not it had a value. This happened when we switch pages in the teacher application then come back to the school select page. However the first time it mounted (and therefore also after refreshing the page) it would load the options correctly and display the value. I added console.logs to determine that indeed, with the default caching, the underlying VirtualizedSelect was not calling our loadOptions callback when it remounted. With caching disabled now, it does.

I'm not worried about perf since this isn't up for long and shouldn't be making too many duplicate calls, the backend sql query is optimized, and we will be debouncing the API call anyway.

@aoby aoby added the bug-fix label Jan 12, 2018
@aoby aoby requested review from Hamms and Erin007 January 12, 2018 02:27
@aoby
Copy link
Contributor Author

aoby commented Jan 12, 2018

… cause it to show blank even when it had a value
@aoby aoby force-pushed the school-autocomplete-fix-remount-blank branch from 9ea1ad3 to dc84f14 Compare January 12, 2018 02:40
@Hamms
Copy link
Contributor

Hamms commented Jan 12, 2018

How difficult would it be to write a test that fully reproduces the original failure case?

@aoby
Copy link
Contributor Author

aoby commented Jan 12, 2018

I initially tried that and ran into some issues, but I didn't try very hard and I think I may have been making a mistake. I'll give it another shot. I agree that would be better.

@aoby
Copy link
Contributor Author

aoby commented Jan 12, 2018

@Hamms I can't get it to repro in a test. When I spy getOptions, then unmount and remount the control, getOptions is (correctly) called twice regardless of the value of the cache prop. I can easily repro manually, so I'm probably not recreating the exact conditions. I don't think this is worth much more time, and I like I said I don't see harm in disabling the cache (I'm pretty sure we didn't even know it existed). Let me know if you have any testing ideas or want to take a look at what I have so far.

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

Thanks for looking! I agree that it's not super necessary, and this change looks just fine without it.

@aoby aoby merged commit 774db2b into staging Jan 12, 2018
@aoby aoby deleted the school-autocomplete-fix-remount-blank branch January 12, 2018 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants