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

Correct domain replacement for GraphQL fields #650

Merged
merged 1 commit into from
Nov 17, 2021

Conversation

apmatthews
Copy link
Member

@apmatthews apmatthews commented Nov 16, 2021

Resolves #541

As mentioned in #541, it appeared we were not doing domain replacement on menu item URLs. Upon investigation, it appears we are performing replacements that should cover menu item URLs, but these replacements were short-circuited by the presence of generalSettings in the response data. I believe the intention here was to skip replacements inside of generalSettings, as queries for generalSettings.url should accurately report the URL of the WordPress instance rather than the front-end application.

With the way GQty assembles queries, it's likely generalSettings will be included in most requests. This PR changes the replacement callback to skip generalSettings without preventing replacements elsewhere in the response data.

Testing

  • Unit tests have been added.

Manual Testing

  1. Before pulling this branch, make a simple menu in Appearance->Menus and assign it to the Primary menu location.
  2. Run the following query in GraphiQL:
query MyQuery {
  generalSettings {
    url
  }
  menuItems(where: {location: PRIMARY}) {
    nodes {
      label
      url
    }
  }
  menus(where: {location: PRIMARY}) {
    nodes {
      menuItems {
        nodes {
          label
          url
        }
      }
    }
  }
}
  1. See that no url fields receive domain replacements
  2. Pull the branch
  3. Run the same query as above
  4. See that generalSettings.url is unchanged, but all menu item urls have been rewritten:
{
  "data": {
    "generalSettings": {
      "url": "http://faustjs.local"
    },
    "menuItems": {
      "nodes": [
        {
          "label": "Hello World",
          "url": "http://localhost:3000/hello-world/"
        }
      ]
    },
    "menus": {
      "nodes": [
        {
          "menuItems": {
            "nodes": [
              {
                "label": "Hello World",
                "url": "http://localhost:3000/hello-world/"
              }
            ]
          }
        }
      ]
    }
  }
}

@apmatthews apmatthews requested a review from a team November 16, 2021 23:10
Copy link
Contributor

@blakewilson blakewilson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@blakewilson blakewilson merged commit 2fe73fe into canary Nov 17, 2021
@blakewilson blakewilson deleted the fix-menu-url-rewrites branch November 17, 2021 21:44
apmatthews added a commit that referenced this pull request Nov 17, 2021
* fix: correct domain replacement for graphql fields (#650)

* docs: correct `dotenv` dep to `dotenv-flow` (#646)

* docs: correct `dotenv` dep to `dotenv-flow`

* docs: replace rest of `dotenv` -> `dotenv-flow`

Co-authored-by: Andrew Matthews <[email protected]>
blakewilson added a commit that referenced this pull request Nov 19, 2021
* fix: correct domain replacement for graphql fields (#650)

* docs: correct `dotenv` dep to `dotenv-flow` (#646)

* docs: correct `dotenv` dep to `dotenv-flow`

* docs: replace rest of `dotenv` -> `dotenv-flow`

* chore: create pr template (#657)

* docs: add "troubleshooting" ref to GQty intro (#656)

* doc: (#519) adding examples for getStaticPaths

* Update API Router auth import docs

* Update API Router imports to use `@faustjs/core/api`

* Breakout Hooks into individual Pages (#631)

* docs: break out hooks

* Update links to appropriate hooks url

* Logging queries guide

* Fix docs site URLs

* Fix hooks links

* docs: GQty introduction

* Revert "docs: GQty introduction"

This reverts commit a952a04.

* docs: intro to GQty

* docs: add troubleshooting ref to GQty intro

Co-authored-by: William Johnston <[email protected]>

* docs: embed Faust.js tutorial walkthrough video (#658)

Co-authored-by: Andrew Matthews <[email protected]>
Co-authored-by: William Johnston <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL rewrites for menus
2 participants