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)