evgenyneu / js-evaluator-for-android

A library for running JavaScript in Android apps.
MIT License
485 stars 84 forks source link

subsequent registered of evaluation is not executed #41

Closed clydzik closed 6 years ago

clydzik commented 6 years ago

If i request com.evgenii.jsevaluator.JsEvaluator#evaluate(java.lang.String, com.evgenii.jsevaluator.interfaces.JsCallback)

few times in short time distance (when previous is not evaluated) the previously scheduled evaluation is not delivered as success or failure

probably reason: loadUrl method of webView is cancelling previously page that is opening in: com.evgenii.jsevaluator.WebViewWrapper#loadJavaScript

evgenyneu commented 6 years ago

Hi @clydzik, thanks for reporting this. You are absolutely right. Unfortunately, this is an issue. When we call the evaluate method, the JavaScript text is sent to the web view for evaluation and it is done asynchronously. If we call the evaluate method again, the new JavaScript text is loaded into the same web view object, since there is only one web view per evaluator. As a result, the previous evaluation is simply discarded without calling the success or the error callback.

Any suggestions on how to improve this are welcome. :)

clydzik commented 6 years ago

ok. when fixing previous 'listeners' i assumed it is working. Sorry for that. Now i'm using this more heavily so i noticed the issue. Now i see two directions. -make it one request by purpose and drop responsibility to 'library user'. shortly cancel any actually evaluations or do not allow new if existing is pending -add a queue and handle it in lib.

what you suggest ?

evgenyneu commented 6 years ago

Good suggestions, as a library maintainer I am biased, since I would prefer a solution that adds the minimal amount of code. :) This is the solution to "drop responsibility to 'library user'". Which is how it works currently.

clydzik commented 6 years ago

hi. sorry for late reply. I understand Your point of view. I still believe this can be done pretty simple with a queue run on evaluate call vs /success/failure calls.

I will try to make a wip PR. Just do not know when. At this point will keep separate instances for my use cases.

So as You wrote. Actually library user is responsible for multiple parallel requests handling

evgenyneu commented 6 years ago

Thanks for your suggestion, but I would be prefer to keep the library code simple (keep as it is). Maybe it's worth adding a note to the readme about it: "the previous JavaScript evaluation is discarded if it is not finished by the time the new evaluation is requested".

clydzik commented 6 years ago

ok. i got Your point. Actually the leak issue i solved will still occur when you do a second call before first go back. So maybe the interface should be cleared to fail fast or drop the list (previous), map (current) implementation that holds callbacks.

simply one callback that is overwritten when second call occur. To clean the interface.

evgenyneu commented 6 years ago

Oh good point. Maybe instead of storing array of callbacks, just have a single instance variable to store a single callback. This way, when new evaluation is requested it overwrites the previous callback. This will make the code simpler and get rid of the leaks automatically. Is it what you were suggesting?

I don't remember why I created the array of callback in the first place, to be honest.

clydzik commented 6 years ago

Yes. Exactly i had this in mind. So i think THIS issue could be closed. Since it is 'work as designed' - lets say. The cleanup. Since i started the 'leak stuff' i may push it forward. From the other hand can leave it to You. Let me know.

Thanks

evgenyneu commented 6 years ago

Yes, feel free to submit the pull request for the leak issue. Thanks.

clydzik commented 6 years ago

hello. sorry for late response. I could not reopen old issue. So continue here the PR: https://github.com/evgenyneu/js-evaluator-for-android/pull/42

let me know what You think

clydzik commented 6 years ago

cool. so i'm closing this as 'work as expected' queue should be implemented on 'client' side

evgenyneu commented 6 years ago

Thanks for your contributions!