-
Notifications
You must be signed in to change notification settings - Fork 6
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
Current state in monitorJob #26
Comments
I had that implemented first (by having |
Ah, wait. Now I understand... the interval doesn't execute the callback at the start. It always waits the interval once... Okay, makes much sense! Should it always run, also if there are no logs? |
Actually I just found that I mistakenly used setTimeout instead of setInterval. See #28 |
Exactly, the interval doesn't execute the callback in the beginning, only after the timeout has passed once. The way I was using it now was:
In this case, the page would not show any logs until the interval has passed once. Of course I could go and fetch the logs myself once but that somehow felt a bit like a duplicate. I don't think it needs to run if there are no logs though, it doesn't really fetch any new information then, right? But do you know if there are any logs before running the |
Here would be my approach: #28 The question is whether it needs to run once just to call describeJob... I guess not? At least createJob also calls it.
Could be. |
Aaah, now I get the point with the |
Yep, looks good from my side and the quick test did exactly what I was looking for. Is this now also according to what you intended with I don't think it has to run the callback just for the sake of calling |
Hmm... I'm back and forth with it. For some use cases it's useful to call describeJob, for some not so much. So I guess I just leave it as it is in the PR, which means the callback is always called once at the beginning with the updated job details. |
I don't think it hurts to run the callback with an updated job and no logs in the beginning, even if this is the exact same job which the user (maybe) already has. So the PR should be fine I think. Which use case would benefit from getting the updated job in any case right away? When you create the Job object directly with |
The use case I'm thinking about is the Web Editor, where the user starts monitorJob maybe just a minute after creating the job. For other use cases where you just start monitoring directly after creating the job, it's likely not getting you a different result. |
Okay, I just implemented the approach that describeJob is only called at the beginning if the data has not been refreshed recently (in the last second) and then it calls the callback if the status changed or logs are available (i.e. older than the time specified in the interval). I think that caters for both use cases best. |
Has been merged. I'll make a rc3 release later today. |
The new version has been pushed to npm. |
Just updated the app to the newest version locally and this works perfectly, thanks a lot! 👍 |
Currently one has to wait the set interval until the first logs are available. Especially when the interval is set to a higher timeout, the user has no feedback at the beginning. This means that if we were to show the logs in a UI somewhere, we have to kind of copy the functionality of the
monitorFn
inmonitorJob
and run it ourselves before startingmonitorJob
to be able to show something until the first timeout.This would work of course but I personally think it would be a nice addition if the
monitorJob
function would take care of this already.What do you think?
The text was updated successfully, but these errors were encountered: