Refactorings #124
Refactorings #124
Conversation
| const XHR = new XMLHttpRequest() | ||
| XHR.open("POST", "") | ||
| XHR.setRequestHeader("Content-Type", "application/json") | ||
| const xhr = new XMLHttpRequest() |
There was a problem hiding this comment.
so i get why you changed this, and that's totally fine, but for some reason most places make XHR capitalized because its an acronym /shrug
There was a problem hiding this comment.
I don't actually know or have an opinion on which it's supposed to be, but was following @bluepichu's comments on #123
There was a problem hiding this comment.
The general convention I've seen (outside the JS API) is that acronyms are single words, and thus get camel-cased as such. So you would have xhr and createNewXhr, for example. We could go a different way if you feel strongly.
| window.getSelection().empty ? window.getSelection().empty() : undefined | ||
| window.getSelection().removeAllRanges ? window.getSelection().removeAllRanges() : undefined | ||
| window.getSelection().addRange(range) | ||
| const $copyBuffer = $("#copy-buffer") |
There was a problem hiding this comment.
I don't like the use of $ to prefix the variable name. What is the rationale? Is it just to indicate that it's a jquery object?
There was a problem hiding this comment.
yeah pretty much, and helps avoid colliding names with existing vars
There was a problem hiding this comment.
ehhhh, I would rather not have it (perhaps because so many languages use that syntax to denote that something is a variable at all). @bluepichu tiebreaker?
There was a problem hiding this comment.
I'd also avoid it unless you feel very strongly.
| @@ -1,59 +1,39 @@ | |||
| "use strict" | |||
|
|
|||
| const denodeify = require("denodeify") | |||
There was a problem hiding this comment.
Does this work? In the past i haven't been able to use denodeify because it was screwing up the prototypical ownership.
There was a problem hiding this comment.
Or would util.promisify work?
| } | ||
| })(handle) | ||
| } | ||
| ApiHandler.routes = Object.freeze({ |
There was a problem hiding this comment.
@bluepichu can I get another pair of eyes on this?
There was a problem hiding this comment.
I think it's fine, but I'm getting myself confused lol
There was a problem hiding this comment.
...I don't get why we need to freeze but it otherwise looks reasonable
| const globals = {} | ||
| globals.coll = undefined | ||
| globals.path = undefined | ||
|
|
||
| function denodeify(fn, args, alt, th) { |
There was a problem hiding this comment.
at the very least I can pull them into utils and probably rename it to avoid confusion
|
Regarding combining the font requests: the CSS file is only cached for 1 day, while the font is cached for a year. Combining them in theory reduces cache hits, but the lifetime is so short it probably doesn't matter. |
|
Interesting, how do you get those numbers? |
|
From here:
|
I don't think this is how pull requests work
ApiHandlerto classapp.useapi/renderer after static resources