Initial comments on implementation

[kadamwhite]

Thanks for taking a stab at this! Some initial thoughts:

endpoint, offset, and password are conventionally written in lower-case, as one word, in existing REST API documentation & clients. username sometimes is, as well.

I've struggled in my own library with whether to use "more idiomatic" camelCased identifiers, vs the snake_cased (e.g. per_page) identifiers. That's a personal call, more than anything, and I ended up doing what you did with perPage, etc.

In terms of the REST API's internal implementation, what you have as wpSystem and apiVersion are actually one single "namespace" string: wp/v2, wc/v1, etc. They are not maintained separately within the API's innards so I was discouraged from making them separate in my own code, and would recommend to unify them here as well.

[seb86]

In terms of the REST API's internal implementation, what you have as wpSystem and apiVersion are actually one single "namespace" string: wp/v2, wc/v1, etc. They are not maintained separately within the API's innards so I was discouraged from making them separate in my own code, and would recommend to unify them here as well.

For the WP core rest API I understand that but what about other developers REST API like WooCommerce, AffiliateWP etc.

[kadamwhite]

Good question. In woocommerce's code it's still going to be just one string, because that's how the route gets registered -- the slashed "namespace/version" is a convention, nothing more. I'm hesitant to recommend artificially separating the two because it seems to increase the number of things you have to worry about when you're using the library, without any clear benefit.

[seb86]

OK. That makes sense. I will change that to one variable. Call it restAPI.

var products = restjQuery({
    siteUrl: "https://wc-rest-testing.dev",
    restAPI: 'wc/v1/',
    endPoint: 'products',
});

[kadamwhite]

I would suggest apiNamespace or namespace; "restAPI" refers to the overall access mechanism, not to a particular version of a subset of endpoints

[seb86]

OK.

var products = restjQuery({
    siteUrl: "https://wc-rest-testing.dev",
    namespace: 'wc/v1/',
    endPoint: 'products',
});

[kadamwhite]

:+1: Also, feel free to ignore my advice at any point -- these are not commandments. Just trying to help keep the wrapper consistent with what it wraps!

[seb86]

Your advice is important @kadamwhite I'm still learning the REST API myself so I want this to be perfect for public use. 👍

[nylen]

I agree with @kadamwhite's feedback above, +1 on namespace. Similarly I would get rid of the combination of endPoint and postID, with the expectation that the client builds this parameter. For comments, for example, this doesn't make much sense, and in order to fix this you'd end up adding a bunch of "magic" string concatenation values.

In general, any string concatenation that's occurring needs to be pretty well-documented, including what happens with trailing and leading slashes at each of the boundaries. IMO there should be as few of these boundaries as possible, and they should match the conventions of the REST (:drum:) of the API. Here is my suggestion for URL parameter structure:

namespace: 'wp/v2', // note no leading or trailing slash; `wp/v2` can be the default
endpoint: '/comments/4', // note leading slash, but this should not be required
...

Regarding parameter casing, I would recommend sticking with snake_case for consistency with the REST API. Here is one example of searching through the WordPress codebase to identify a convention:

~/wp/wordpress-develop-official $ ack -iw endpoint | grep -io endpoint | sort | uniq -c
    132 endpoint
     28 endPoint
     27 Endpoint
      3 EndPoint

All instances of endPoint are inside the third-party TinyMCE code in src/wp-includes/js/tinymce/tinymce.js and refer to cursor selection, so that can basically be ignored.

[nylen]

A couple more bits of feedback on naming (which is one of the two hardest problems in computer science, apart from cache invalidation and off-by-one errors):

offSet, userName, and passWord are usually written as one word so shouldn't need a capital letter or an underscore.

If you allow the client to pass through query parameters without trying to modify the case used by the API, then the need to provide pageNumber, perPage, and offSet parameters goes away entirely. Your documentation could then just state "this library accepts any of the standard filter parameters used by the WP REST API [link here]", and you shouldn't need to do anything to handle them in your code.

[seb86]

@nylen So the query parameters are just part of the endpoint, correct?

[nylen]

See:

• https://developer.wordpress.org/rest-api/using-the-rest-api/global-parameters/
• https://developer.wordpress.org/rest-api/using-the-rest-api/pagination/
• https://core.trac.wordpress.org/browser/tags/4.7.3/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L145