Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Create a constants.js file with the NAMESPACE value #799

Merged
merged 3 commits into from
Aug 7, 2019

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Aug 2, 2019

This PR creates a constants.js file as discussed here: #779 (comment).

How to test the changes in this Pull Request:

Creating a new post, adding the affected blocks and verifying they still fetch information should be enough to test.

Affected blocks:

  • Featured Category
  • Products by Attribute
  • Products by Category
  • Featured Product

Changelog

Internal: create a NAMESPACE constant and export it from assets/js/constant.js.

@Aljullu Aljullu added status: needs review type: refactor The issue/PR is related to refactoring. labels Aug 2, 2019
@Aljullu Aljullu requested a review from a team August 2, 2019 14:31
@Aljullu Aljullu self-assigned this Aug 2, 2019
Copy link
Member

@mikejolley mikejolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the namespace for the API actually wc/blocks? wc/blocks/products is an endpoint - and there is no guarantee we won't have other endpoints using the wc/blocks namespace in the future.

@nerrad
Copy link
Contributor

nerrad commented Aug 2, 2019

Isn't the namespace for the API actually wc/blocks? wc/blocks/products is an endpoint - and there is no guarantee we won't have other endpoints using the wc/blocks namespace in the future.

This is a good point. With that in mind, I wonder if it'd be better to create an ENDPOINTS or ROUTES constant instead that is an object returning the endpoint strings. So you could have:

const ENDPOINTS = {
    root: 'wc/blocks',
    products: 'wc/blocks/products'
};

That retains flexibility for future additional endpoints?

Also, an education type question I have... are these endpoints part of a versioned api? If not, should they be? When I do a request to /wp-json on my local test site I see there are two routes related to blocks in the output namespaces; wc-blocks/v1 and wc/blocks. Is the wc-blocks/v1 namespace for blocks bundled with WooCommerce and wc/blocks reserved for the feature plugin?

@Aljullu Aljullu added this to the 2.4 milestone Aug 2, 2019
@mikejolley
Copy link
Member

@nerrad since it's a private API (now) we don't need to version--blocks is the only consumer on this API. The core version is being removed in 3.7.

@Aljullu
Copy link
Contributor Author

Aljullu commented Aug 7, 2019

Thanks for the reviews @mikejolley @nerrad. I replaced the NAMESPACE constant with the ENDPOINTS object @nerrad suggested.

This PR is ready for another look.

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pre-approving because it's fine as is, but I do have a minor tweak to my original suggestion that I think is worth considering.

export const ENDPOINTS = {
root: '/wc/blocks',
products: '/wc/blocks/products',
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I know this is following my initial suggestion but I'm wondering if we should do something like this instead:

const NAMESPACE = '/wc/blocks';
export const ENDPOINTS = {
    root: NAMESPACE,
    products: `${ NAMESPACE }/products`,
};
  • reduces chance of typo errors for the namespace on future endpoints added.
  • easier swapping out of namespace (albeit search and replace is arguably just as easy).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f727482.

@Aljullu Aljullu merged commit 4543eae into master Aug 7, 2019
@Aljullu Aljullu deleted the add/create-constants-file branch August 7, 2019 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: refactor The issue/PR is related to refactoring.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants