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
feat: Experiments API #3098
base: develop
Are you sure you want to change the base?
feat: Experiments API #3098
Conversation
@justlevine I like where this is going. Will try and look at it more detailed shortly, but very nice idea |
@jasonbahl this is very early work, I'll ping you when it's worth your time to look over for real |
9245c75
to
fec4324
Compare
269ff74
to
c5ec8ed
Compare
Code Climate has analyzed commit d51f9c0 and detected 0 issues on this pull request. View more on Code Climate. |
@jasonbahl this is ready for initial review.
|
/** | ||
* Whether the experiment is active. | ||
*/ | ||
public function is_active(): bool { |
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.
I see the phpDoc for some methods are missing @return
. Is this intentional?
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.
Yes, if the return type hint is identical to the signature
/** | ||
* Prepares the configuration. | ||
* | ||
* @return array{title:string,description:string} |
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.
Are we using phpstan
now?
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.
We've been using it for a while. I'll probably unseal the type before final merge, but for now it makes sure nothing unnecessary is leaking in to the PR
*/ | ||
protected function is_enabled(): bool { | ||
if ( ! isset( $this->is_enabled ) ) { | ||
$this->is_enabled = defined( 'GRAPHQL_EXPERIMENTAL_FEATURES' ) ? (bool) GRAPHQL_EXPERIMENTAL_FEATURES : true; |
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.
Is there any benefit in adding a filter hook
option here?
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.
Here specifically I'm not sure about, since the intent of the check here is to allow a user to intentionally disable the entire Experimental module.
We might want a filter in AbstractExperiment
beyond what graphql_get_setting()
provides in order to allow for codependent Experiments without overloading the entire is_active()
method 🧐
*/ | ||
protected function is_enabled(): bool { | ||
if ( ! isset( $this->is_enabled ) ) { | ||
$this->is_enabled = defined( 'GRAPHQL_EXPERIMENTAL_FEATURES' ) ? (bool) GRAPHQL_EXPERIMENTAL_FEATURES : true; |
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.
Do we have any custom tab in the Site Health screen/page? Might help with debugging.
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.
Not yet, but it was mentioned in the last Office Hours 🤞
if ( ! isset( self::$registry ) ) { | ||
_doing_it_wrong( | ||
__METHOD__, | ||
esc_html__( 'Registered experiments have not been set. Make sure not to call this function before the `graphql_experiments_registered` hook.', 'wp-graphql' ), | ||
'@todo' | ||
); | ||
|
||
return []; | ||
} |
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.
Very educational. 🎈
|
||
$is_active = 'on' === get_graphql_setting( $setting_key, 'off', Admin::$option_group ); | ||
} | ||
|
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.
Per @renatonascalves other comment, this could be a good spot for a filter, e.g. if Experiment A requires Experiment B to be active.
|
||
return []; | ||
} | ||
|
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 multiple experiments are registered with the same slug we should call that out.
When testing the API, I accidentally left the slug the same as the test experiement, and my new one showed up but the test one disappeared and it took me a second to realize that it was because I left the slug the same. I caught it quickly, but would have caught it quicker if something warned me about the duplicate slugs.
@justlevine What is the difference between an "Experiment" and a "Feature Plugin"? In both cases I have the option to activate / deactivate some functionality. But the difference is that experiments are now bundled in core WPGraphQL where feature plugins (i.e. WPGraphQL Smart Cache, etc) can be versioned, updated, rolled-back, etc separate from WPGraphQL core. I'm curious what you believe the scope of an "Experiment" should be and how it is different than another plugin that can be installed and activated? Some examples of possible experiments that come to my mind might be:
I don't actually know how to justify bundling these in core as an experiment vs. shipping them as a separate plugin that folks can "enable" via installing and activating the plugin 🤔 |
I think the main difference from a feature plugin is that you can activate a feature per request, per part of the site. Similar to this plugin. Instead of deactivating the whole plugin. Experiment here, to me, is similar to an A/B test feature. I need both code to be available/active but I can choose to use one at a time or at the same time per request/page/etc. |
What do you mean "per request"? I read that as per http request. Like one query might have the feature enabled and one query might not? Or something like that? Does the HTTP request have to identify that it wants to use said experiment via query parameter or something? I guess I'm not fully following |
@renatonascalves looking at the plugin you shared from Alley, I'm still not sure how that differs from activating/de-activating a plugin. The user experience is still:
That sounds just like the user experience of navigating to the plugins page and using a checkbox interface to activate a plugin. I think I need some more concrete examples and use cases of where this makes sense and adds value that an extension plugin doesn't make sense or doesn't add the same value? It's clear there are use cases, but I'm struggling to see them |
I think I'll leave for @justlevine to explain it since it is his feature. But I understood the feature as something that could be toggled on and off easily and used or not per request/page/location (but that is more about how the feature could be used rather than the intention of the feature). |
From @justlevine: End user statement:"WPGraphQL Experiments allow you to experience the future of WPGraphQL today". Purpose:Allow for the shipping, iterating, and gathering feedback on (potential) upcoming changes to WPGraphQL core. What makes a good experiment:
What makes a bad experiment:
Real world triage:
|
@justlevine how would we migrate an experiment to production or deprecate an experiment we believe was a mistake? |
To tl;dr my above notes:
With the above context some replies:
With the above clarification between feature plugin and extension, the justification is reversed and usually clearer.
Similar but not exactly. I think I addressed most of this already so tl;dr for now, but happy to expand if needed:
Moving on from use case to implementation:
I havn't implemented this into the code yet, but per my mental model:
|
@justlevine how do consumers of the API know that they are consuming field(s) / Type(s) that are experimental? For orgs / teams where there are different people/teams consuming the API and another person/team maintaining the WordPress install, it would be nice for a consumer of the API to know whether they are interacting with something experimental and/or explicitly ask to interact with an experimental version of the API or a stable version. Hypothetical Scenario:WP team enables an experimental feature xx. Client team starts consuming xx. Experiment is deprecated. An admin notice was displayed for the WordPress team, but the client team didn't know about the impending deprecation. Client app is now breaking. Should we consider outputting something in the Extensions output of GraphQL queries to indicate active experiments? And/or allow the client to pass something in the request like: graphql( [
'query' => $query,
'variables' => $variables,
'extensions' => [
'experimentsEnabled' => true/false // or something along those lines?
]
] ); And/or the payload of a response could include something like: {
"data": {
"someExperimentalField": "some data",
},
"extensions": {
"enabledExperiments": [
"experimentOne": {
"description": "Description of the experiment and impact on the Schema / Resolution",
"link": "https://github.com/wp-graphql/path/to/docs/about/the/experiment.md",
"notes": "Maybe some notes such as impending deprecation date or impending core merge date or other?"
}
]
}
} This would allow clients to have some insight into things that may/may not be around in the future, etc 🤔 |
@jasonbahl I think a simple
(More so, I think a Anything beyond that though feels like scope creep:
|
What does this implement/fix? Explain your changes.
This PR introduces the new Experiments API, allowing us to begin working/iterating on on experimental features within WPGraphQL core without worrying about future compatibility.
How it Works.
WPGraphQL\Experimental\Experiment\AbstractExperiment
class.WPGraphQL\Experimental\ExperimentRegistry
, and their functionality loaded (by callingAbstractExperiment::init()
) only if the experiment is active.WPGraphQL Settings > Experiments
tab, or programmatically with theGRAPHQL_EXPERIMENTAL_FEATURES
constant.More details in
src/Experimental/README.md
Tasks
Does this close any currently open issues?
Part of #3081
Any relevant logs, error output, GraphiQL screenshots, etc?
Note
This screen recycles existing
Settings API field types.
To improve the UX we may want to introduce a new field type callback.
Any other comments?
src/Experimental/Experiment/TestExperiment
. This should be removed before the PR is merged.Where has this been tested?
Operating System: Ubuntu 20.04 (wsl2+ devilbox + php 8.1.15)
WordPress Version: 6.5.2