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

fix: performance issues for mapping field groups to the Schema #152

Merged
merged 13 commits into from Jan 19, 2024

Conversation

jasonbahl
Copy link
Contributor

@jasonbahl jasonbahl commented Jan 11, 2024

What does this implement/fix? Explain your changes.

This addresses some performance issues related to mapping the ACF Field Groups to the Schema.

Currently there's logic in place that tries to determine what GraphQL Types in the schema should have a field to access the ACF Field Group from by parsing the field group's "location rules".

This logic is expensive and slow and already runs in the ACF Admin UI when creating/updating field groups, storing the types as the graphql_types field on the ACF Field Group.

Instead of running the location mapping at Schema Generation, we should rely on the field groups already defining a "graphql_types" field.

Plugin Build

Here's a built version of the plugin with these changes: wpgraphql-acf.zip

Does this close any currently open issues?

no (issue discussed in Slack)

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

I used an acf-json directory provided by a user. The config generated 234 Object Types implementing the AcfFieldGroup Interface.

Before

Trying to load the GraphQL Schema would take, well, a REALLY long time.

CleanShot 2024-01-11 at 15 02 31

And executing a simple query such as:

{
  posts {
    nodes {
      id
      title
    }
  }
}

would also take a LONG time:

CleanShot 2024-01-11 at 15 05 55

😱 😱 😱

AFTER

Now the Schema, with all 234 AcfFieldGroup Types, loads MUCH faster:

CleanShot 2024-01-11 at 15 10 27

And the simple posts query also loads MUCH faster:

CleanShot 2024-01-11 at 15 09 49

Any other comments?

This MIGHT be a breaking change for some users that don't have graphql_types defined on their ACF Field Groups.

Need to investigate this a bit more.

For users that have graphql_types defined on their ACF Field Groups this will work already.

chore: prep for launch on the .org repo
… not try and dynamically determine location rules at Schema Generation time.
@jasonbahl jasonbahl added the Compat: Possible Break There's a possibility this might lead to breaking changes, but not confirmed yet. label Jan 11, 2024
@jasonbahl jasonbahl self-assigned this Jan 11, 2024
…r field groups that have graphql_types defined, but still support field groups that don't have graphql_types defined.
@josephfusco josephfusco self-assigned this Jan 17, 2024
… Field Group Location rules to reduce redundancy

- remove unneeded LocationRules
- update OptionsPageTest to clear the schema during setUp
…phQL\Utils\Utils

- normalize location rules to be mapped with strolower
@jasonbahl jasonbahl closed this Jan 18, 2024
@jasonbahl jasonbahl reopened this Jan 18, 2024
…gistered to the Schema

- Use the registered field names and keys to determine wether or not to resolve the field from the post itself (if revision meta is supported) or the parent (if it's not).
Copy link
Member

@josephfusco josephfusco left a comment

Choose a reason for hiding this comment

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

LGTM! I was able to see a significant performance improvement applying these changes. Much better than before!

@jasonbahl jasonbahl merged commit 3b34a89 into develop Jan 19, 2024
25 checks passed
@jasonbahl jasonbahl mentioned this pull request Jan 23, 2024
@jasonbahl jasonbahl deleted the fix/performance-issues branch January 24, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compat: Possible Break There's a possibility this might lead to breaking changes, but not confirmed yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants