Skip to content
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

Replace hard-coded API version with global config #3042

Closed
wants to merge 1 commit into from

Conversation

georgeh
Copy link
Contributor

@georgeh georgeh commented Oct 17, 2017

Description

This changes editor components to use the global wpApiSettings.versionString instead of hardcoding "wp/v2/". This is the same approach used in lib/client-assets.php

How Has This Been Tested?

Tested by creating a post and confirming it published correctly.

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@georgeh georgeh requested a review from youknowriad October 17, 2017 19:08
@aduth
Copy link
Member

aduth commented Oct 17, 2017

Can you explain the rationale here? Would this automatically opt consumers in to a newer version if one were implemented, thus having potential for breakage? Can we be doing better than to call out to the global settings, maybe by passing through as an argument or inferring it (omitting altogether)? If it needs to be overridden externally, perhaps there's another way to achieve this (filters, callbacks) than to put the burden on the component implementer?

@georgeh
Copy link
Contributor Author

georgeh commented Oct 17, 2017

Hi! Our goal is to use Gutenberg with a different REST API URL. We have been able to get that working with these changes to the versionString and the rest_url hook to change the URL base for API calls.

Would this automatically opt consumers in to a newer version if one were implemented, thus having potential for breakage?

Yes. If a newer version of the API were to go live and change the versionString, we would automatically start using the new one. Using the global versionString seems to be a common pattern so I expect that any future breaking changes will offer deprecation ahead of time.

Can we be doing better than to call out to the global settings, maybe by passing through as an argument or inferring it (omitting altogether)? If it needs to be overridden externally, perhaps there's another way to achieve this (filters, callbacks) than to put the burden on the component implementer?

I looked at trying to do this externally, potentially with APIProvider, but it looks like it's importing all of its options from the global scope, which gets us back to where we started.

Is there was a way for a plugin to inject a request filter into WithAPIDatathat could recognize, say, GET /wp/v2/users/me?context=edit and turn it into a request we want?

If not, what would that look like? I'm imagining lib/client-assets.php loading a script through a filter that declares a global off of the wp namespace with the methods WithAPIData needs to make a network request. That way a plugin could change the object to use any old transport.

That seems more future-proof but adds a bunch of indirection and work without adding any value to our specific use case.

@mtias mtias added Core REST API Task Task for Core REST API efforts Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts labels Oct 18, 2017
@aduth
Copy link
Member

aduth commented Oct 19, 2017

adds a bunch of indirection and work without adding any value to our specific use case.

Is it not in the interest of exploring if it keeps the implementation simpler for the consumer of withAPIData?

A few more general concerns:

  • withAPIData is an independent component, and there is no guarantee that the wpApiSettings global is even available
  • If we needed to, there's already a pattern of passing a few "helpers" as a second argument to the mapPropsToData for withAPIData. We use this for e.g. post type and taxonomy mappings. This might be a more appropriate place to surface additional arguments if necessary.
  • While this applies to the wp/v2 namespace, is this a generalized solution for any endpoint namespace? Or do all namespaces need to be transformed to the different URL structure?

As far as enabling overrides, we could follow the pattern of the APIProvider's context function, which already passes values from the wpApiSettings global:

// APIProvider
//
// - context.getAPISchema
// - context.getAPIPostTypeRestBaseMapping
// - context.getAPITaxonomyRestBaseMapping
[
APIProvider,
{
...wpApiSettings,
...pick( wp.api, [
'postTypeRestBaseMapping',
'taxonomyRestBaseMapping',
] ),
},
],

Alternatively, we could explore a pattern for on-request filtering, perhaps using the newly-published (and revisions-pending) hooks module.

@georgeh
Copy link
Contributor Author

georgeh commented Nov 27, 2017

#3377 changed the versionString so we are taking a different path

@georgeh georgeh closed this Nov 27, 2017
@georgeh georgeh deleted the update/api-version branch November 27, 2017 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants