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

perf: refactor PluginConnectionResolver to only fetch plugins once #3084

Conversation

justlevine
Copy link
Collaborator

What does this implement/fix? Explain your changes.

This PR refactors our PluginConnectionResolver class to use a DRY ::get_all_plugins() method for building the list of available plugins.
This list is then stored in a class property, so the fetching, filtering, etc only occur once.

Does this close any currently open issues?

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

Part of #2749

Any other comments?

This PR is based on #3082 , which should be merged first.

Where has this been tested?

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

WordPress Version: 6.4.3

@justlevine justlevine added Type: Enhancement New feature or request Status: In Review Needs to be reviewed by a project maintainer before merge Needs: Reviewer Response This needs the attention of a codeowner or maintainer. Component: Connections Issues related to connections ObjectType: Plugin labels Mar 30, 2024
@jasonbahl jasonbahl merged commit 7857764 into chore/connection-resolvers-cleanup Apr 1, 2024
2 checks passed
@justlevine
Copy link
Collaborator Author

justlevine commented Apr 1, 2024

Ps @jasonbahl it might be easier for you to review/merge #3082 first, (PRs based on it, should auto-update the branch to develop) instead of merging the dependent PRs into #3082.

From my side: I don't know your availability for reviewing them all, but assuming it's a multi-day process, then merging the parent PRs before the dependents would also let me continue backporting more things while decreasing the risk of merge conflicts.

Either way

@justlevine justlevine deleted the dev/cr2-backport/refactor-plugin-connection-resolver branch April 1, 2024 21:20
@jasonbahl
Copy link
Collaborator

This one didn't look like it had any direct connection to the others and could be merged as-is.

The others did seem to have a lot more correlation.

@justlevine
Copy link
Collaborator Author

@jasonbahl this was merged into #3082 , not into #3097 . We probably should amend the release notes.

@jasonbahl
Copy link
Collaborator

@justlevine ooph. Good call. Will update. Thanks. Just got the test results back and this was another case of the "moving too fasts"

@jasonbahl
Copy link
Collaborator

@justlevine edited the release notes: https://github.com/wp-graphql/wp-graphql/releases/tag/v1.23.0

This was referenced Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Connections Issues related to connections Needs: Reviewer Response This needs the attention of a codeowner or maintainer. ObjectType: Plugin Status: In Review Needs to be reviewed by a project maintainer before merge Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants