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

Implement support for annotating certain plugins as experimental #1111

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

felixarntz
Copy link
Member

Summary

Fixes #1045

Relevant technical choices

  • Introduces a new perflab_get_standalone_plugin_data() function so that all standalone plugin data can be defined in one place.
    • I'm sure we wouldn't want to have to maintain the plugin list in two places, e.g. one for whether each plugin is experimental or not, and another one for each plugin's constant.
  • Marks auto-sizes and embed-optimizer as experimental for now.
    • auto-sizes because its browser behavior isn't really available yet, embed-optimizer because it's very new and a 0.x version.
  • Displays an "(experimental)" annotation on the feature card behind every plugin that is marked experimental.
  • Displays the experimental plugins after all the non-experimental plugins.

See screenshot of what this would look like based on the PR implementation:
Screenshot 2024-04-03 at 5 57 44 PM

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure Creating standalone plugins labels Apr 4, 2024
@felixarntz felixarntz added this to the performance-lab 3.0.0 milestone Apr 4, 2024
Copy link

github-actions bot commented Apr 4, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@felixarntz
Copy link
Member Author

Note for reviewers: I have opened this against release/3.0.0 since it's a technically relatively simple change which IMO would be great to include if we can get it across the finish line (we're still 1.5 weeks away from that release).

If there's substantial feedback or further discussion required that means this can't land in 3.0.0, we can adjust the base branch and change it to target 3.1.0.

Copy link
Member Author

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Another note: The CI failures in https://github.com/WordPress/performance/actions/runs/8547642712/job/23420147133?pr=1111 are unrelated, so we should fix them separately. See #1012.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @felixarntz for the PR. It look good to me.

@mukeshpanchal27
Copy link
Member

Another note: The CI failures in https://github.com/WordPress/performance/actions/runs/8547642712/job/23420147133?pr=1111 are unrelated, so we should fix them separately. See #1012.

The PR is #1112 not 1012

@swissspidy
Copy link
Member

I was hoping we can just mark them as experimental based on the version number (i.e. < 1.0.0 is experimental), but turns out Auto Sizes is already at 1.0.0 🙃

@felixarntz
Copy link
Member Author

@swissspidy

I was hoping we can just mark them as experimental based on the version number (i.e. < 1.0.0 is experimental), but turns out Auto Sizes is already at 1.0.0 🙃

Yeah, we probably need an extra flag. It's also worth considering that in certain edge-cases we may even want to designate plugins as experimental again when they weren't before. Potentially this could apply to Dominant Color Images / Image Placeholders as right now nobody is really exploring a path forward or maintaining it, and it received a lot of concerns/pushback when initially proposed in a Make Core post. Just thinking out loud, I'm not saying we should do that. But probably a flag is helpful for several reasons.

Could you (or someone else) please give another review to this PR? Since #1112 was now merged into this, the CI failures are resolved, so this is good for a review.

*/
function perflab_get_standalone_plugin_version_constants() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for reviewers: This function is simply moved down (see bottom of the file), it's not being replaced as potentially the diff here may suggest.

…s are explicitly recommended, some are experimental).
load.php Outdated Show resolved Hide resolved
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM but with a suggestion to formalize the description of a return value with an array shape for static analysis.

Co-authored-by: Weston Ruter <westonruter@google.com>
@felixarntz felixarntz merged commit a408134 into release/3.0.0 Apr 4, 2024
15 checks passed
@felixarntz felixarntz deleted the add/1045-experimental-plugin-annotations branch April 4, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants