Separation Frontend and Backend Functionality in NDT Client

shredtechular commented 8 years ago

This PR is the first step at separating out the Frontend and Backend functionality that exists today in the NDT client as discussed in https://github.com/m-lab/m-lab.github.io/issues/102 on the MLab website. The idea is to ease how an implementer might include an NDT js library onto their website and by including only the necessary js, an implementer could create their own frontend using the NDT js library.

As the current implementation would require a website to include five different scripts, this PR starts to address that breaking up the script.js file into frontend and backend functionality. This removes the dependency for an NDT js library to need jQuery, the gauges.min.js library, or the script.js file. Those files would ONLY be needed if a user wanted the NDT frontend that exists on the site today, but not if it was creating a custom frontend.

Other implementation notes:

If we can determine how best to combine the 3 backend scripts, and implementer could simply include 1 js script and then write their own frontend using the library of existing methods against the newly created NDT object. For example, to trigger the start of a test, a developer can now simply write NDT.startTest() in their own frontend code. Similarly, to get the status of a test, a developer can now simply write NDT.testStatus(), and etc.
made one minor tweak in the ndt-browser-client.js file as that function is called in multiple places and since self is referring to last created object doesn't always work.
added more documentation in both the code that remained in the script.js file

Items this PR does NOT address:

[ ] Combining the ndt-wrapper.js, ndt-browser-client.js, and newly created ndt-library.js into one concatenated/min file. This enhancement would be a new PR, but we need to decide if it is best to
1. create a build script to do this - I'd lean towards doing this, however, not sure if we do this, where does this library build and get deployed? Additionally, if other repos are using git submodules, are those sites expected to build this as well or just link to a place where the js is deployed? Should the build script be included in the parent repo that includes this code via submodule?
2. manually concatenate these js files into one file - Simpler solution, however, not as clean codebase obviously.
[ ] Combining the jquery, gauges.min.js, and script.js files into one file or separating these into it's own "frontend" repo. Not sure if this is even desired, but something to think about optimally implementing.
[ ] Updates to the "widget.html" file. This file seems pretty different than what is on the mlab website and not sure if there is one that is right or wrong so not sure what would be best to proceed on this. For example, should we just update this widget to point to the newly separated js? Also, I think this can be fixed once it is decided on how scripts may be concat/min above.

Other Enhancements that could be done here:

[ ] Some documentation clearly identifying the methods and tools available with the NDT js library so implementers know how to use it. Maybe this can be generated with jsDocs given scripts are following proper formats. I imagine additional detail may be needed here though. Also, need to figure out where this should live? Within this repo or external?
[ ] Enhance the "monitoring" of a test so it is easier for implementers to get test results. A couple areas in the script.js still have un-obvious chaining that probably wouldn't be easy for implementers. I can tweak this a bit more to get this to be a little friendlier, but wanted to get a PR going to start to ensure we are on the right track with the approach.
[ ] I wonder if some of the "monitoring" sections in the script.js could also be re-worked to utilize templates/binding instead of hardcoding html in there.
[ ] Is there a use case for a user passing in a specific server? If so, then maybe we could make that more dynamic as well and over-ride what is being returned in the ajax call to get the backend server to run tests against.
[ ] Introduce unit test framework and unit tests for this library

This change is

mtlynch commented 8 years ago

This is fairly large for a PR. Would it be possible to break this up into multiple PRs? I see a potential breakup like

Refactoring into ndt-library.js (and updating references)
Refactoring code in script.js beyond (1)
Adding documentation to existing functions that needed it

It looks like you're doing this already, but just want to make sure we're going by the Google JS Style Guidelines. This isn't a Google project, but it doesn't look like the original code has any consistent style guidelines applied and the M-Lab team is most familiar with the Google JS style guide.

I love the added documentation on functions that didn't have it. Thanks for adding that!

Previously, shredtechular (Andrew Newhouse) wrote…

> #### Separation Frontend and Backend Functionality in NDT Client > > This PR is the first step at separating out the Frontend and Backend functionality that exists today in the NDT client as discussed in https://github.com/m-lab/m-lab.github.io/issues/102 on the MLab website. The idea is to ease how an implementer might include an NDT js library onto their website and by including only the necessary js, an implementer could create their own frontend using the NDT js library. > > As the current implementation would require a website to include [five different scripts](https://github.com/m-lab/m-lab.github.io/pull/76/files#diff-56ff23ff6448049981daee3c0344f9cdR22), this PR starts to address that breaking up the [script.js](https://github.com/m-lab/ndt/blob/master/HTML5-frontend/script.js) file into frontend and backend functionality. This removes the dependency for an NDT js library to need jQuery, the gauges.min.js library, or the script.js file. Those files would **ONLY** be needed if a user wanted the NDT frontend that exists on the site today, but not if it was creating a custom frontend. > > Other implementation notes: > - If we can determine how best to combine the 3 backend scripts, and implementer could simply include 1 js script and then write their own frontend using the library of existing methods against the newly created `NDT` object. For example, to trigger the start of a test, a developer can now simply write `NDT.startTest()` in their own frontend code. Similarly, to get the status of a test, a developer can now simply write `NDT.testStatus()`, and etc. > - made one minor tweak in the ndt-browser-client.js file as that function is called in multiple places and since self is referring to last created object doesn't always work. > - added more documentation in both the code that remained in the script.js file > > Items this PR does **NOT** address: > - [ ] Combining the ndt-wrapper.js, ndt-browser-client.js, and newly created ndt-library.js into one concatenated/min file. This enhancement would be a new PR, but we need to decide if it is best to > 1. create a build script to do this - > > I'd lean towards doing this, however, not sure if we do this, where does this library build and get deployed? Additionally, if other repos are using git submodules, are those sites expected to build this as well or just link to a place where the js is deployed? Should the build script be included in the parent repo that includes this code via submodule? > 2. manually concatenate these js files into one file - > > Simpler solution, however, not as clean codebase obviously. > - [ ] Combining the jquery, gauges.min.js, and script.js files into one file or separating these into it's own "frontend" repo. Not sure if this is even desired, but something to think about optimally implementing. > - [ ] Updates to the "widget.html" file. This [file](https://github.com/m-lab/ndt/blob/master/HTML5-frontend/widget.html) seems pretty different than what is on the [mlab website](https://github.com/m-lab/m-lab.github.io/blob/master/_layouts/ndt-test.html) and not sure if there is one that is right or wrong so not sure what would be best to proceed on this. For example, should we just update this widget to point to the newly separated js? Also, I think this can be fixed once it is decided on how scripts may be concat/min above. > > Other Enhancements that could be done here: > - [ ] Some documentation clearly identifying the methods and tools available with the NDT js library so implementers know how to use it. Maybe this can be generated with jsDocs given scripts are following proper formats. I imagine additional detail may be needed here though. Also, need to figure out where this should live? Within this repo or external? > - [ ] Enhance the "monitoring" of a test so it is easier for implementers to get test results. A couple areas in the script.js still have un-obvious chaining that probably wouldn't be easy for implementers. I can tweak this a bit more to get this to be a little friendlier, but wanted to get a PR going to start to ensure we are on the right track with the approach. > - [ ] I wonder if some of the "monitoring" sections in the script.js could also be re-worked to utilize templates/binding instead of hardcoding html in there. > - [ ] Is there a use case for a user passing in a specific server? If so, then maybe we could make that more dynamic as well and over-ride what is being returned in the ajax call to get the backend server to run tests against. > - [ ] Introduce unit test framework and unit tests for this library > >

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

HTML5-frontend/ndt-library.js, line 6 [r1] (raw file):

 * utilize.  The NDT object is self invoked when the library is included on a
 * page, however, the frontend can customize certain properties when the
 * interaction is initialized with the startTest method.

Trailing whitespace on this line. This seems to repeat a few times in the PR. Can we grep to catch all of them?

HTML5-frontend/ndt-library.js, line 13 [r1] (raw file):


/**
 * Immediately Invoked Function Expression that creates NDT library object

Only first word should be capitalized

HTML5-frontend/ndt-library.js, line 95 [r1] (raw file):

   * Creates the backend for the NDT object.  Tries to create websocket backend
   * if supported, falls back to java applet if websocket not available
   * @param - none

Does JSDoc require us to document lack of a parameter? (ditto throughout)

HTML5-frontend/script.js, line 504 [r1] (raw file):


/**
 * Parses, formats, and displays final summary of diagnostic information for NDT test

Can we limit comments to 80 columns?

Comments from Reviewable

shredtechular commented 8 years ago

I have broken this PR into 2 PRs to hopefully more clearly show the changes being made and why.

- 1 PR that purely takes some of the backend functionality out of script js and creates the new ndt-library.js file from it. It does also contain changes to script.js and minor fixes to a couple other files in order to make this standalone PR work completely.
1 PR that cleans up the script.js a little more adding comments, removing unnecessary code, etc. (dependent on the first PR and will need to be rebased)

Also, to answer your question, yes, I was definitely trying to follow the Google JS Style Guide, but I think you did find a few things that I missed.