search

flosell / lambdacd

a library to define a continuous delivery pipeline in code
https://www.lambda.cd/
Apache License 2.0
677 stars 59 forks source link

Reduce number of ajax requests from ui #187 #188

Closed alehatsman closed 6 years ago

alehatsman commented 6 years ago

I fixed the problem with req rate. While fixing I noticed that re-frame version is too old, so I updated version, fixed problems with new version etc.. Sorry that everything in one pr, otherwise I had to do it two times...

Please review the code!

alehatsman commented 6 years ago

Ok, a new version of React uses Map data structure, so we need to include polyfill or use another js env, for example, chrome headless instead of phantomjs.

flosell commented 6 years ago

Hi, thanks for the PR! I might not get around to reviewing this in detail until the weekend though. The tests are failing because of the new react version, right? I'm OK with switching to another js env for testing, I think the whole thing can use a revamp. I don't have strong opinions on the topic, so if you want to take this on, go wild! :) If this is too much effort, and you just want to fix the AJAX request problem I'm also OK with going back to the old versions in this PR.

alehatsman commented 6 years ago

So, I changed js env to chrome-headless. Clojurescript tests are ok. I see some errors in clj code which are not related to my changes and which I can't reproduce locally. :/ Trying to figure out what is it about...

alehatsman commented 6 years ago

I have triggered it in my fork and all builds are successful. Maybe the problem is in the cache.

alehatsman commented 6 years ago

About ./go serve-cljs update of figwheel to latest version helped, seems like old figwheel was using his own clojurescript version and that version didn't support :global-exports which new cljsjs/react is using.

flosell commented 6 years ago

Looks good to me so far. Figwheel is working as well :)

The pipeline state updates still look like they have the old behaviour and the the title still has the [WIP]-tag so I'm assuming you are still working on things.

Let me know when you are ready and we'll get this merged and released.

alehatsman commented 6 years ago

I just deployed that version to test on my real cd env if will not figure out any issues we can merge.

"state updates still look like they have the old behaviour", can u explain what do u mean?

flosell commented 6 years ago

Ok, thanks!

"state updates still look like they have the old behaviour", can u explain what do u mean?

Looking at the code again I think I was wrong there :) What I was referring to was that only the update-history handlers set the update-in-progress? flag so I thought only update-history was covered by the feature. But since the ticker is reading it, update-history and update-pipeline-state are covered.

This might become a problem when the update-pipeline-state call is a lot slower than the update-history call (this can happen if the pipeline produces massive amounts of logs so there's a lot more data to read from the pipeline state). In this cases requests might still queue up.

The PR is already a good improvement so I'm happy to merge it as-is but if you want to improve it more, this would be one suggestion.

flosell commented 6 years ago

Ah, and one more thing: Could you add an entry in the CHANGELOG.md to let others know this has been fixed? (You might need to add the section for the next release)

alehatsman commented 6 years ago

Feels like we are good to go!

flosell commented 6 years ago

Looks good to me, merging now and releasing afterwards. Thanks for the feedback and thanks for all the work you put into this!