Convert clblob console.html from jQuery to Mithril.js #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also use === and !== for more predictable comparisons instead of == and != The "dot notation" warnings were not fixed as array-style access provides more clarity here. Those warnings can be suppressed with: /* jshint sub:true */
Also, store the clusters and by_ip in globals. This is to make it easier to convert the individual functions to Mithril vdom generated from a well-defined model.
Create stateForReplica global for tracking replica state, and make support functions to query it and update it. Use stateForReplica instead of CSS classes to store information about the replicas (including warning level and whether they should be hidden). The use of maximumWarningSeverity may need more work and testing relative to when replica_status is set and when CSS is styled. Ideally that global should be removed.
Remove loading the jQuery library too.
…iming This fixes an issue where sometimes warning level could be out-of-date due to it being used before the replica drawing routines calculated it using a replica_status global. This fix is inefficient because it essentially draws replicas an extra time on updates -- and ideally it could be rethought in the process or removing the replica_status global which is modified as a side effect of rendering.
Consistently use double-quote (") instead of single-quote (').
Remove quotes where not needed in summary object definition.
Also fix missing space in metric function.
Remove typos. Configure JSHint warnings.
Looks more like Python :-) Related: https://mislav.net/2010/05/semicolons/
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The complex clblob console single-page application is hard to understand and extend in jQuery. This PR improves that situation by converting console.html to use Mithril.js instead. This is also a proof-of-concept for using Mithril in other Craigslist operations.
=== More details
Mithril.js is a vdom library similar to React but lighter-weight, hopefully reflecting Craigslist's minimalist design approach. While React is more popular, it has a bigger footprint, is somewhat slower, and most importantly comes with the Facebook Patents clause as a potential risk.
The advantage of Mithril over jQuery here is to make a single-page application easier to understand, maintain, and enhance. Previously, jQuery was used to store application state in HTML and CSS which makes application state harder to reason about. After the conversion, there is now essentially a model of the application state (as globals) which is re-rendered on every change from a UI action, network request, or timeout. Behind the scenes Mithril.js optimized the re-rendering to minimize DOM updates.
The Mithril vdom approach is similar to how many video games are implemented. It is much easier to reason about how state turns into display items through a one-way conversion algorithm than it is to reason about how display items get modified here and there by snippets of code (some of which are even in strings passed to event handlers) which store application state scattered throughout the DOM.
Every commit here results in a runnable application -- although there was a CSS update issue introduced in the middle which was fixed later. Most commits are small steps except for one big one near the end. Retaining the commit history rather than squashing this into one commit makes the conversion process easier to follow as a learning experiences about how to convert other jQuery-based applications to Mithril.
Currently this code loads Mithril from a CDN. In production Mithril should be loaded locally for security reasons, but I was not sure how Craigslist would need this done since jQuery is defined in python-clcommon and not in this repo. The simplest approach is just to add Mithril 1.1.1 to this repo.
There is a TODO item remaining for optimization. The code still retains using the replica_status global which is modified as a side-effect of the metric rendering function to determine a maximum warning severity level for the replica. The simplest upgrade approach was to keep that global for now and call part of the rendering process twice on request status updates, The rendering for the replica is called once to calculate the severity but the rendering itself is not used -- then the entire document is re-rendered with the correct maximum warning severity for the replica. This works, but it could be better.
A "nohide" CSS class and an "ok" CSS class was introduced into the output to represent default states for convenience of implementation and future styling, but otherwise the output should (ideally) be identical to the jQuery version. Those classes could be removed if desired with a little more work.
Future work might include splitting out the JavaScript code into a separate file, moving it into TypeScript to more clearly define the data model as a single class instance instead of a set of globals, and perhaps changing the CSS class approach to use Tachyons-css.
Obviously a big change like this would need a lot more testing before being used in production. I have only tested it on a developer machine with the "examples" cluster.conf file and without any significant file activity. Ideally there would be unit and functional test for this UI code. Mithril makes it possible to check fragments of vdom output against a standard in unit tests without having to actually generate DOM nodes in a real or phantom browser. But this is about all I have time for at the moment. That would also involve picking a testing framework (like Mocha) and introducing it into the repository.
I can't quite say "it hovers much better now" because the console (by intent) still does essentially exactly what it did before -- it only works very differently under the hood in a non-visible way. So rather than upgrading to a hover car (yet), this change is like upgrading from a 2000s Toyota to a 2010s Tesla -- it has a small learning curve but it is hopefully worth it for ultimately lower operating and maintenance costs.
The big payoff of this conversion is in making it easier for the console application to "hover" in the future, like by:
There are of course many vdoms out there. A list of vdoms I put together last year can be found here for comparison:
dojo/meta#11 (comment)
Mithril.js is just my favorite of all those for a combination of the implementation, the licensing, the primary author's design style, and the community. The documentation for Mithril.js can be found here: https://mithril.js.org