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

feat: Experiments API #3098

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

justlevine
Copy link
Collaborator

@justlevine justlevine commented Apr 11, 2024

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.

  1. Individual experiments are implemented by extending the WPGraphQL\Experimental\Experiment\AbstractExperiment class.
  2. These classes are initialized by the WPGraphQL\Experimental\ExperimentRegistry, and their functionality loaded (by calling AbstractExperiment::init() ) only if the experiment is active.
  3. Users can toggle experiments from the WPGraphQL Settings > Experiments tab, or programmatically with the GRAPHQL_EXPERIMENTAL_FEATURES constant.

More details in src/Experimental/README.md

Tasks

  • Scaffold API
  • Create example Experiment.
  • Review and improve Admin UI
  • Implement tests
  • Add usage/developer docs

Does this close any currently open issues?

Part of #3081

Any relevant logs, error output, GraphiQL screenshots, etc?

  1. Experiments Tab

Note

This screen recycles existing Settings API field types.
To improve the UX we may want to introduce a new field type callback.

image

  1. When active, the new field is added to the schema

image

Any other comments?

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2+ devilbox + php 8.1.15)

WordPress Version: 6.5.2

@justlevine justlevine added Status: In Progress Type: Feature scope: API Issues related to access functions, actions, and filters labels Apr 11, 2024
@coveralls
Copy link

coveralls commented Apr 11, 2024

Coverage Status

coverage: 83.29% (-1.0%) from 84.289%
when pulling d51f9c0 on justlevine:feat/experiments-api
into a66f3e1 on wp-graphql:develop.

@jasonbahl
Copy link
Collaborator

@justlevine I like where this is going. Will try and look at it more detailed shortly, but very nice idea

@justlevine
Copy link
Collaborator Author

@jasonbahl this is very early work, I'll ping you when it's worth your time to look over for real

Copy link

codeclimate bot commented Apr 27, 2024

Code Climate has analyzed commit d51f9c0 and detected 0 issues on this pull request.

View more on Code Climate.

@justlevine justlevine added Status: Discussion Requires a discussion to proceed Needs: Reviewer Response This needs the attention of a codeowner or maintainer. and removed Status: In Progress labels Apr 27, 2024
@justlevine
Copy link
Collaborator Author

justlevine commented Apr 27, 2024

@jasonbahl this is ready for initial review.

  • Ideally the Admin UX would be improved, but that will require a new Settings API field type callback.
  • Waiting to have feedback before tests get implemented.
  • The TestExperiment is super barebones, but considering you can do pretty much whatever you want inside ChildExperiment::init(), i'm not entirely sure if we need anything else.

/**
* Whether the experiment is active.
*/
public function is_active(): bool {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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}
Copy link
Collaborator

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?

Copy link
Collaborator Author

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;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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;
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 🤞

Comment on lines +76 to +84
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 [];
}
Copy link
Collaborator

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 );
}

Copy link
Collaborator Author

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 [];
}

Copy link
Collaborator

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.

@jasonbahl
Copy link
Collaborator

@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:

  • "Offset Pagination": letting users opt-in to page-based pagination instead of (or alongside) cursor pagination
  • "Custom Scalars": Enable certain custom scalars and/or associate them with certain fields
  • "Activity Tracking": WPGraphQL Smart Cache has "activity tracking" (similar to WPGatsby) that pays attention to events in WordPress and centralizes when to emit a "purge" action. I could see this functionality coming to core WordPress and maybe enabled via an Experiment while we polish things?

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 🤔

@renatonascalves
Copy link
Collaborator

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.

@jasonbahl
Copy link
Collaborator

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.

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

@jasonbahl
Copy link
Collaborator

jasonbahl commented May 8, 2024

@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:

while logged in to the WordPress admin as an administrator (or a user with the manage_options capability) you can turn feature flags on and off via a simple checkbox interface

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

@renatonascalves
Copy link
Collaborator

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).

@jasonbahl
Copy link
Collaborator

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:

  • limited in scope
  • changes or enhances fundamental behavior of WPGraphQL core
  • is greenlit as a core feature on a roadmap level, but would either be a breaking change to implement, or needs more feedback about whether it's actually helpful for the bulk of our users/iteration before it can be recommended in production.
  • can be supported by maintainers.

What makes a bad experiment:

  • Features that only benefit a small subset of users (e.g. WP plugin interop)
  • Features that we aren't planning to implement into core (e.g. offset pagination)
  • Features that require more than just iteration before they can be considered usable in production.
  • Large complex features that would develop better in an isolated environment.
  • Features that are ready on our side, but require ecosystem adoption to prevent breaking changes downstream (e.g. Blocks)
  • Features we're unable unwilling to actively maintain.

Real world triage:

  • custom scalars > experiment
  • @OneOf directive / input rewrite > experiment
  • GraphiQL 2.0 > experiment
  • Offset Pagination > plugin
  • ACF > Plugin
  • Smart Cache > probably plugin with parts in core, but it's a bit gray
  • Gutenberg > experiment
  • Tax Query > experiment if this is something you eventually want to support in core, otherwise plugin.
  • Widgets/Sidebars > experiments if we want to see if people actually use this enough to include in core.

@jasonbahl
Copy link
Collaborator

@justlevine how would we migrate an experiment to production or deprecate an experiment we believe was a mistake?

@justlevine
Copy link
Collaborator Author

justlevine commented May 8, 2024

To tl;dr my above notes:

  1. Experiments are future flags, not feature flags. They allow us to work on greenlit features/enhancements for WPGraphQL core that for a myriad of possible considerations (breaking changes, ecosystem conficts, needs feedback/polish to make it forward-compatible/work at scale) aren't ready for mass adoption.
    Experiments are supported and maintained in their current version, but there is no guarantee of back/forward-compatibility. Similarly, they may be removed without notice.

  2. Features that service a small subset of users or provide an alternative to WPGraphQL core behavior are Plugin Extensions. Support and development of those Extensions are offered by those extensions.

  3. Feature Plugins are philosophically similar to Experiments, but practically similar to Extensions. Before this API we had no choice but to iterate outside of WPGraphQL core. With the API, there are fewer good candidates for a separate Feature Plugin, which likely that meet a combination of the following criteria:

    • A feature that is currently at the "idea phase" and not MVP-complete
    • A feature that would benefit from independent iteration and testing outside of the traditional WPGraphQL cadence.
    • A feature that we're unable/unwilling to provide the same level of support for as WPGraphQL core code at its current stage of development.
    • A feature that needs a project-level decision on whether to greenlight something for core.

With the above context some replies:

@jasonbahl

"Offset Pagination": letting users opt-in to page-based pagination instead of (or alongside) cursor pagination
❌ It's an alternative experience for a subset of users and one we don't plan to actively support. If in the future we wanted to allow all users to paginate by either offset or cursor, then we could Experiment in core until we're ready to ship the API publicly.

"Custom Scalars": Enable certain custom scalars and/or associate them with certain fields
✔️ Including them means both us and 3rd pary plugins can start building on them before we're ready to break the entire schema by turning them on publicly.

"Activity Tracking": WPGraphQL Smart Cache has "activity tracking" (similar to WPGatsby) that pays attention to events in WordPress and centralizes when to emit a "purge" action. I could see this functionality coming to core WordPress and maybe enabled via an Experiment while we polish things?
✔️ While parts of Smart Cache make sense to stay in the plugin, an Activity Tracking API is indeed a good candidate for core adoption (applies to the entire eco, greenlit by @jasonbahl , etc). Plugins like Smart Cache would force-enable the Experiment, and then build around it like other core WPGraphQL features.

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 🤔

With the above clarification between feature plugin and extension, the justification is reversed and usually clearer. Experiment for core changes, Plugin Extension for alternative behavior that doesn't meet the 80/20 rule, Feature Plugin when practical concerns about support/development justify the extra complexity for both developers and users (who need to find out about and install the plugin before they can activate it).

@renatonascalves

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.

[...] I understood the feature as something that could be toggled on and off easily and used or not per request/page/location

Similar but not exactly. I think I addressed most of this already so tl;dr for now, but happy to expand if needed:

  • As mentioned, future flags are not feature flags. The goal is to use them to iterate more quickly on WPGraphQL core functionality.
  • While there is a benefit to the user of being able to easily toggle a feature on/off without having to install/activate a separate plugin, that's more of a happy side effect. The intent is to be able to test, iterate, and allow extensions/end users to opt into functionality with significantly less overhead.
  • Experiments could be used for A/B testing, but from a WPGraphQL core perspective: E.g. a totally new DataLoader that we want to make sure has performance parity with the existing DataLoader. They are not intended to allow for different requests to behave differently IRL.

Moving on from use case to implementation:

@jasonbahl

how would we migrate an experiment to production or deprecate an experiment we believe was a mistake?

I havn't implemented this into the code yet, but per my mental model:

  • Experiment Graduation:

    1. we relocate the code outside of experiments into the actual Core classes.
    2. (optional) we display a dismissible admin notice letting a user know when an activated Experiment has graduated.
    3. we display a (dismissible?) admin notice letting a user know they no longer need to include the experiment slug in the constant, if they're programmatically activating the experiment.
    4. We update the Admin UX to grey out the toggle and say the experiment is graduated. We remove the experiment from the list altogether in the next breaking release.
  • Experiment Deprecation/Removal

    1. If/when deprecated, a dismissable admin notice warning about the impending remove will appear every X days.
    2. When removed, a permanent admin notice warns that the experiment has been removed for those who had it it activated before update or those who are programmatically activating it.
    3. We update the admin UX to grey out the toggle and say the experiment is no longer available. We remove the experiment from the list altogether in the next breaking release.
    4. (Optional) Depending on the Experiment, whims of the maintainers, or whatever we may drop the code into an extension or code snippet.
    5. (Based on parallel conversation with @jasonbahl on Discord): We check for removed active experiments during our future SemVer update checker (Idea: SemVer auto-update enforcement #2543)

@jasonbahl
Copy link
Collaborator

@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 🤔

@justlevine
Copy link
Collaborator Author

justlevine commented May 8, 2024

@jasonbahl I think a simple graphql_debug() notice on deprecation or removal would be a great idea: e.g.

The % Experiment has been deprecated and will be removed in v%. Please [migrate your code](change notice link) and deactivate the Experiment, or you will no longer be able to update WPGraphQL.

(More so, I think a graphql_debug() when WPGraphQL can't update bc of a compatibility issue would make a great addition to #2543 🤯)

Anything beyond that though feels like scope creep:

  • The frontend doesn't need debug data on active experiments, same as about any specific backend configuration setting. Schema is the source of truth and we can convey what's necessary_if anything_ via type name prefixes or descriptions, or even an empty Experimental interface, but I think the place to consider that is when we implement our first real experiment, and likely per experiment. There's a separate discussion to be had about exposing Site Health data, but imo that needs to happen holistically and away from the context of experiments.

  • More generally, I just don't believe the hypothetical scenario is a real user expectation. It's commonly understood that changing WordPress's configuration (deactivating a plugin, changing the url structure, removing a CPT, renaming an ACF field group) will alter the schema, and manually activating/deactivating an experiment is no different. And like all other WP configs they're not getting toggled much beyond initial setup . What we should be worried about is a change that isn't caused by user action, i.e. deprecation/removals/breaking updates. The grapql_debug() message covers that.

  • Exposing active experiments as a schema field however is additively useful. Possibly can be used to @skip (not sure offhand, I rarely need directives ), surely can be used in userland to conditionally use a different fragment or query in order to support both core and experimental behavior. This is a framework/library use case though, so I've left all the data exposure to be handled by graphql_register_setting() for now so we can focus on the consumer API first and then think about extension/framework implementations and repurposing in subsequent PRs. Easier to iterate w.o breaking changes this way IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Reviewer Response This needs the attention of a codeowner or maintainer. scope: API Issues related to access functions, actions, and filters Status: Discussion Requires a discussion to proceed Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants