List OAuth applications under their local sites
Review Request #9094 — Created July 20, 2017 and submitted
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. |
reviewbot | |
I'm not a fan of the shorthand syntax, especially here where you could just create the object all in one … |
david | |
Doc comment? |
david | |
This is ugly. How about: const templateOptions = { emptyText: (localSiteName ? interpolate(localSiteEmptyText, [localSiteName]) : emptyText), }; Note also that … |
david | |
Can we have the positive case first? |
david | |
Doc comment? |
david | |
Can you add Model Attributes: docs for these? |
chipx86 | |
id is implicitly defined for models. No need to put it in here. |
chipx86 | |
Can we use options here? Otherwise the docs aren't going to generate right when we get around to adding that. |
chipx86 | |
Indentation is off. |
chipx86 | |
for .. of creates a huge amount of JavaScript (with multiple layers of exception handling, nested loops, and several layers … |
chipx86 | |
Can this be combined? |
chipx86 | |
Same as above. |
chipx86 | |
"a raw" |
chipx86 | |
Blank line between these. |
chipx86 | |
Maybe put the [ on the previous line, so these two lines can just be pure list items and on … |
chipx86 | |
We're still doing 2 loops when I'm sure we can do just 1. What's stopping us from using a standard … |
chipx86 | |
(A lot of this comment is about the under-the-hood operations of iterators and how Babel transpiles stuff, so take it … |
chipx86 | |
Can we wrap this as: Object.entries(options.apps) .map(...) Right now it reads as if we're doing some operation on Object instead … |
david | |
These would fit on one line, which would let you dedent the block by 4 spaces. |
david |
-
-
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}/`, });
-
-
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 beenemptyText
. -
-
-
-
-
Can we use
options
here? Otherwise the docs aren't going to generate right when we get around to adding that. -
-
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 asfor .. 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? -
-
-
-
-
-
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. -
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?
-
-
(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:
- Use
Map.forEach
, which is going to be nicer than building a throw-away array or using Babel'sfor .. of
. - 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. - Use