-
Notifications
You must be signed in to change notification settings - Fork 21
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 ConfigLoader
with deduplicated logging and try_examples
config caching
#249
base: main
Are you sure you want to change the base?
Add ConfigLoader
with deduplicated logging and try_examples
config caching
#249
Conversation
ConfigLoader
with throttled logging and try_examples.json
config cachingConfigLoader
with throttled logging and try_examples
config caching
I hope this is in line with what you had in mind, @Carreau! :) I used a local Screen.Recording.2025-01-10.at.7.45.16.AM.mp4To compare with the previous functionality, this deployment: https://output.circle-artifacts.com/output/job/048f37f1-2a97-4826-b310-3c66a77deb9c/artifacts/0/doc/build/html/api/skimage.feature.html generates thirty errors, one per button. |
f1bb4ea
to
af7f8cd
Compare
Thanks, @Carreau, yes, it makes sense to just remove the throttling. I did it based only on your suggestion, but it feels redundant, especially since we have just one configuration file per iframe/component. I'll remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me – but again I'm not a JS expert.
ConfigLoader
with throttled logging and try_examples
config cachingConfigLoader
with deduplicated logging and try_examples
config caching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was asked to review this pull request last week.
It's nice to see the inefficiency of making several concurrent network requests for the same data being addressed here, so thank you!
The changes introduced in this pull request look good to me, however I do have a couple questions and one concern.
I wasn't able to dive deep into issue #240, but one thing that immediately jumped out at me in that issue's description was:
I assume this happens because we run the JavaScript file per iframe
I just want to be clear that if this JavaScript file is getting loaded into multiple iframes on the same page, this PR will not prevent multiple network requests.
Another thing that caught my attention was the use of the word "retry" in the PR description and in the linked issue. As far as I can tell, there is no retry happening anywhere. To me the word retry means an intentional re-request when you know that the first request failed. I see no logic anywhere in this codebase that does that. I suspect that the word was meant in a different way, but because I'm not super familiar with the issue at hand, I'm bringing it up because I want to make sure I'm not missing something.
These questions bring me to my one big concern which is the lack of tests. In my mind, this is quite a bit of logic to leave untested. Furthermore, tests could go a long way to clarifying intent. For example, if these changes were intended to work across iframes, a good test case would show it working across multiple iframes.
Since the code has already been approved, I am not going to request changes, but lack of tests really worries me for long-term maintainability.
Description
try_examples.json
configuration file are deduplicated, and thetryExamplesConfigLoaded
flag is preserved but managed more carefully.configLoadPromise
has been introduced to handle concurrent requests with a basic cache that prevents multiple retries and prevents console noise.Closes #240
Additional context
In light of scikit-image/scikit-image#7644 where this problem was first noticed.