• 
      

    List OAuth applications under their local sites

    Review Request #9094 — Created July 20, 2017 and submitted

    Information

    Review Board
    release-3.0.x
    f36bf09...

    Reviewers

    Previously, all OAuth2 applications were listed in a single list, giving
    no distinction to those limited to Local Sites. Now they are grouped
    by Local Site so they can be more easily be distinguished.

    This change does not add support for creating applications under Local
    Sites via the UI.

    Testing done:

    • Created and deleted applications on the global site.
    • Deleted applications on a Local Site.

    Description From Last Updated

    Col: 39 Missing semicolon.

    reviewbotreviewbot

    I'm not a fan of the shorthand syntax, especially here where you could just create the object all in one …

    daviddavid

    Doc comment?

    daviddavid

    This is ugly. How about: const templateOptions = { emptyText: (localSiteName ? interpolate(localSiteEmptyText, [localSiteName]) : emptyText), }; Note also that …

    daviddavid

    Can we have the positive case first?

    daviddavid

    Doc comment?

    daviddavid

    Can you add Model Attributes: docs for these?

    chipx86chipx86

    id is implicitly defined for models. No need to put it in here.

    chipx86chipx86

    Can we use options here? Otherwise the docs aren't going to generate right when we get around to adding that.

    chipx86chipx86

    Indentation is off.

    chipx86chipx86

    for .. of creates a huge amount of JavaScript (with multiple layers of exception handling, nested loops, and several layers …

    chipx86chipx86

    Can this be combined?

    chipx86chipx86

    Same as above.

    chipx86chipx86

    "a raw"

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Maybe put the [ on the previous line, so these two lines can just be pure list items and on …

    chipx86chipx86

    We're still doing 2 loops when I'm sure we can do just 1. What's stopping us from using a standard …

    chipx86chipx86

    (A lot of this comment is about the under-the-hood operations of iterators and how Babel transpiles stuff, so take it …

    chipx86chipx86

    Can we wrap this as: Object.entries(options.apps) .map(...) Right now it reads as if we're doing some operation on Object instead …

    daviddavid

    These would fit on one line, which would let you dedent the block by 4 spaces.

    daviddavid
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    brennie
    david
    1. 
        
    2. Show all issues

      I'm not a fan of the shorthand syntax, especially here where you could just create the object all in one go. How about:

      return _.defaults(rsp, {
         apiURL: (localSiteName
                  ? `/s/${localSiteName}${baseURL}${rsp.id}`
                  : `${baseURL}${rsp.id}/`),
         editURL: `${baseEditURL}/${rsp.id}/`,
      });
      
    3. Show all issues

      Doc comment?

    4. reviewboard/static/rb/js/accountPrefsPage/views/oauthApplicationsView.es6.js (Diff revision 2)
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      This is ugly. How about:

      const templateOptions = {
          emptyText: (localSiteName
                      ? interpolate(localSiteEmptyText,
                                    [localSiteName])
                      : emptyText),
      };
      

      Note also that your second case had localSiteEmptyText where I think it should have been emptyText.

    5. Show all issues

      Can we have the positive case first?

    6. 
        
    brennie
    david
    1. 
        
    2. Show all issues

      Doc comment?

    3. 
        
    brennie
    chipx86
    1. 
        
    2. Show all issues

      Can you add Model Attributes: docs for these?

    3. Show all issues

      id is implicitly defined for models. No need to put it in here.

    4. Show all issues

      Can we use options here? Otherwise the docs aren't going to generate right when we get around to adding that.

    5. Show all issues

      Indentation is off.

    6. Show all issues

      for .. of creates a huge amount of JavaScript (with multiple layers of exception handling, nested loops, and several layers of temporary function calls) when going through Babel, totalling about 64 lines of code just for the loop itself. As nice as for .. of would be if native, it's not worth it when transpiling, and I'd really like to avoid using it in all code until we can use it natively.

      Can this be reworked to loop over this with a standard for loop? Looks like we're just looping over an array?

    7. reviewboard/static/rb/js/accountPrefsPage/views/oauthApplicationsView.es6.js (Diff revision 4)
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Can this be combined?

    8. Show all issues

      Same as above.

    9. 
        
    brennie
    chipx86
    1. 
        
    2. Show all issues

      "a raw"

    3. Show all issues

      Blank line between these.

    4. Show all issues

      Maybe put the [ on the previous line, so these two lines can just be pure list items and on the natural indentation level. That'll also help fix the attributes, which are currently indented 1 space too far.

    5. Show all issues

      We're still doing 2 loops when I'm sure we can do just 1. What's stopping us from using a standard dictionary object mapping site names to arrays and doing a standard iteration through it?

    6. 
        
    brennie
    chipx86
    1. 
        
    2. Show all issues

      (A lot of this comment is about the under-the-hood operations of iterators and how Babel transpiles stuff, so take it as an analysis of things, with my recommendation at the very end.)

      This is still more wasteful than needed. We're building an array and discarding it, and requiring a function invocation per entry.

      The cheapest way, performance and memory-wise, would be to use a standard dictionary object with a standard loop:

      this.collections = {};
      
      ...
      
      for (const localSiteName in this.collections) {
          if (this.collections.hasOwnProperty(localSiteName)) {
              const collection = this.collections[localSiteName];
      
              ...
          }
      }
      

      We save memory by not building a throw-away array. We also technically save a little memory by using an object instead of a Map (which has additional bookkeeping), but shrug, not a big deal. It's just the array that I want to get rid of, since it's completely unnecessary and will just be populated and then immediately scheduled to be garbage-collected.

      There are two other options we should look at:

      1. Use Map.forEach, which is going to be nicer than building a throw-away array or using Babel's for .. of.
      2. Use a standard for loop, working with the iterator. A bit messier, probably not really worth it, but just to show how this would look (and how iterators work under the hood):

      for (let entries = m.entries(), entry = entries.next(); !entry.done; entry = entries.next()) {
          const localSiteName = entry.value[0];
          const collection = entry.value[1];
      
          ...
      }
      

      Not as nice as a for .. of (I can't wait until we can fully depend on that in browsers, but until then...)

      Side note on under-the-hood technicals: My code snippet pulled out the items from the entry.value array manually instead of using:

      const [localSiteName, collection] = entry.value;
      

      Every time you do that, Babel injects the following function into your file:

      var _slicedToArray = function() {
          function sliceIterator(arr, i) {
              var _arr = [];
              var _n = true;
              var _d = false;
              var _e = undefined;
              try {
                  for (var _i = arr[Symbol.iterator](), _s; !(_n = (_s = _i.next()).done); _n = true) {
                      _arr.push(_s.value);
                      if (i && _arr.length === i) break;
                  }
              } catch (err) {
                  _d = true;
                  _e = err;
              } finally {
                  try {
                      if (!_n && _i["return"]) _i["return"]();
                  } finally {
                      if (_d) throw _e;
                  }
              }
              return _arr;
          }
          return function(arr, i) {
              if (Array.isArray(arr)) {
                  return arr;
              } else if (Symbol.iterator in Object(arr)) {
                  return sliceIterator(arr, i);
              } else {
                  throw new TypeError("Invalid attempt to destructure non-iterable instance");
              }
          };
      }();
      

      For the case above, since it's dealing with an array, this expands to:

      var array = (function() {
          if (isArray(entry.value)) {
              return entry.value;
          }
      
          // Not reached
      )();
      
      var array = entry.value;
      var localSiteName = array[0];
      var collection = array[1];
      

      Not as bad as it could be, certainly. Still wasteful, but not horribly so. Just means the function gets injected into each file, an additional two function calls per loop, and an additional state variable within the loop. I get the tradeoff of the new syntax being more convenient, but it's tradeoff. That said, I don't particularly mind this as much, but it very much depends on the code being used. Expensive code/long loops probably shouldn't use it.

      I don't know how this looks when bundling -- I suspect we still will get a thousand copies of this function, instead of Babel graciously providing a single version. If we only got one, that would be super nice -- our JS files are getting big as-is).

      Anyway, in summary, I think the best bet for the code here is to use Map.forEach, since each option will result in a per-loop function call somewhere.

    3. 
        
    brennie
    david
    1. Just a couple formatting nits:

    2. Show all issues

      Can we wrap this as:

      Object.entries(options.apps)
          .map(...)
      

      Right now it reads as if we're doing some operation on Object instead of on the contents of options.apps

    3. Show all issues

      These would fit on one line, which would let you dedent the block by 4 spaces.

    4. 
        
    brennie
    david
    1. Ship It!
    2. 
        
    brennie
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (ee31cda)