FLUID-6090: Provide a state tracking component #797
Conversation
First checkin.
Fixed name of test html file.
Added a test to show limitation of the tracker: if the changes occur more frequently than the polling frequency, some changes are missed. Also, modified the html file such that the test checkbox is visible.
Minor cleanup of code and comments.
Changed argument name to be more in line with infusion terminology.
Added unit tests to the main test task.
Added another unit test, and did some code gardening.
Merge upstream master branch into FLUID-6090.
| fluid.stateTracker.startTracking = function (that, changeEvaluator, changeListener, interval) { | ||
| var monitor = that.initMonitorInfo(changeEvaluator); | ||
| that.events.onStateChange.addListener(changeListener); | ||
| if (!!interval) { |
There was a problem hiding this comment.
For every JavaScript value which is truthy, !! of the value is also truthy, and similarly for falsy values, so this double negation isn't necessary - https://developer.mozilla.org/en-US/docs/Glossary/Truthy
There was a problem hiding this comment.
Okay. I had thought the double negative was a sanctioned Crockford-ism, but I can't recall where it's from at the moment. Certainly grunt lint didn't object ;-)
| } | ||
| }); | ||
|
|
||
| // Initiate polling. |
There was a problem hiding this comment.
Could you write these comments in the JavaDoc-like style which we have standardised on in new work (lots of the core framework still lags), e.g. as seen in https://github.com/GPII/universal/blob/master/gpii/node_modules/gpii-oauth2/gpii-oauth2-authz-server/src/AuthorizationService.js#L182
| // that has a function named evaluateChange(). | ||
| // Provided by client. | ||
| // @return - the monitor object. | ||
| fluid.stateTracker.initMonitorInfo = function (changeEvaluator) { |
There was a problem hiding this comment.
A clearer pattern would have the changeEvaluator defined as an invoker on the component itself, with the initial value "fluid.notImplemented" in order to signal that the user needs to override to make the component usable - see https://issues.fluidproject.org/browse/FLUID-5733 for description
There was a problem hiding this comment.
I had considered making the evaluator a function even when this code was part of the ProcessReporter. It was just easier to make it an object that contained whatever other pieces of data or functions the client wanted to stick in there. The unit tests go so far use a fluid component - see https://github.com/klown/infusion/blob/FLUID-6090/tests/component-tests/stateTracker/js/StateTrackerTests.js#L28
Another way to go is to make the state tracker component a base abstract grade with an evaluateChange invoker defined as fluid.notImplemented, as you suggest. Then, clients would derive a concrete grade and add any data/functionality they want.
Is that what you are suggesting here? A base abstract grade?
|
|
||
| fluid.defaults("fluid.stateTracker", { | ||
| gradeNames: ["fluid.component"], | ||
| events: { |
There was a problem hiding this comment.
The component would be more usable if this field were modelised - as well as widening the contract of the "changeEvaluator" function to simply return any value which is then applied to the model. A change in that value in the opinion of the data binding system would then notify any modelListeners bound to the fie.d
There was a problem hiding this comment.
All right, I've made it a fluid.modelCompnent.
|
Cheers, @klown, this is a great useful start on this interesting implementation. It would be helpful before we carry on to have an inventory taken of all potential users of this feature to ensure that we are meeting their needs - and to write up the results of the inventory on the JIRA. I know that both @simonbates and @colinbdclark might have potential use cases, as well as that originally found in the "Process Reporter". We will need at least 2 distinct use cases occurring in the wild in order for this component to make its way into the core framework. |
Addressed some of Antranig's comments: - removed double negative "!!". - java-doc-ized the comments.
State tracker is now an abstract grade (fluid.stateTracker).
The State tracker is now a fluid.modelComponent.
Updated StateTracker to fluid version 3.0.0.
Modified the StateTracker to retain and handle the timer identifier (intervalID) so that clients don't need to. Also, refactored the unit tests.
Fixed a couple of typos.
Call stopTracking() function when the component is destroyed to make sure that the polling is stopped (that the interval timer is cleared).
|
ok to test |
Also, fixed some typos.
Modified startTracking() to check if already tracking, and, if so, to ignore the request and not call setInterval() an additional time.
Refactored to have a clean setup and teardown of the tracker instance.
…e tracker. - Merge upstream master infusion branch into FLUID-6090. - Add code to state tracker html test file to send coverage data - Fixed typo in label element's id reference to the test checkbox
Re-started jqUnit earlier in the asynchronus unit tests.
Unit tests still need work as they fail intermittently.
|
ok to test |
|
CI job failed: https://ci.fluidproject.org/job/infusion-pull-request/25/ |
|
|
amb26
added
the
mothballed
|
Mothballed since this facility will not be developed further within the GPII but might be of further interest if the requirement arises again |
|
Mothballing is fine. This was effectively mothballed anyway. The last time I looked, the tests were failing because they used setInterval() and/or setTimeout() to simulate changing the state of a checkbox -- time to come up with another approach, but not right now. Thanks. |
State tracker component:
https://issues.fluidproject.org/browse/FLUID-6090