New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Metapi multiple urls #63
Conversation
Initial setup to allow caching / getting of multiple reposrts in 1 call. May want to switch to "POST" instead of "GET": easier to pass params, allows for more urls
@ArtOfCode-: |
metapi/metapi.js
Outdated
else if (typeof ident === "number") { | ||
fetchUrl = "https://metasmoke.erwaysoftware.com/api/posts/" + ident + "?key=" + key + optionString; | ||
else if (typeof ids[0] === "number") { | ||
fetchUrl = "https://metasmoke.erwaysoftware.com/api/posts/" + ids + "?key=" + key + optionString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ... + ids + ...
actually work, or do you need to semicolon-join it first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArtOfCode- See line 134 ;-)... oooh, derp.
metapi/metapi.js
Outdated
callback(new metapi.Response(true, items[0])); | ||
if (items.length > 0) { | ||
for (var k = 0; k < items.length; k++) { | ||
metapi.postCache.add(items[k].link, items[k], {overwrite: true}); // Overwrite: "urls to be loaded" aren't cached. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bear in mind that the link
field may not be present in the response. It's probably better to cache it against the ident it was passed to the function with, somehow.
Also, let jQuery handle construction of parameters.
metapi/metapi.js
Outdated
@@ -237,3 +248,47 @@ window.metapi = {}; | |||
sock.addCallback(messageCallback); | |||
}; | |||
})(); | |||
|
|||
metapi.getPost( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a testing method, it could do with being removed before merge :)
|
||
var overwrite = options.hasOwnProperty("forceReload") && delete options["forceReload"]; | ||
var overwrite = options.hasOwnProperty("forceReload") && options["forceReload"]; | ||
delete options["forceReload"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not tack this onto the end of the previous condition?
options.hasOwnProperty("forceReload") && options["forceReload"] && delete options["forceReload"];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it won't delete the option if it's false
. The option always has to be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, true
metapi/metapi.js
Outdated
for (var j = 0; j < ident.length; j++) { | ||
var cached = metapi.postCache.get(ident[j]); | ||
if (cached && options.page === 1 && !overwrite) { | ||
response.push(new metapi.Response(true, cached)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add a new metapi.Response
to the response array for each post; ideally, we have one metapi.Response
containing every post.
(promise I'm not nitpicking, I'd just like this to be as good as possible :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I already did that for the pagination handling, just this case wasn't quite right.
metapi/metapi.js
Outdated
|
||
var f = options.filter; | ||
if (f && (!f.included_fields || f.included_fields.indexOf(requiredFilter) === -1)) { | ||
callback(new metapi.Response(false, "An invalid filter is passed in the options.")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The metapi.Response
constructor needs an object as the second parameter, containing error_name
, error_code
, and error_message
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArtOfCode-: Line 347 does callback(new metapi.Response(false, jqXhr.responseText));
, so I figured it'd work here as well.
What error_code
s should I use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, jQuery parses jqXhr.responseText
into an object. Probably 400? HTTP Bad Request seems equivalent.
Allows the loading of multiple posts from a single metapi call.