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

[gatsby-source-wordpress] Error in includedRoutes documentation ? #10871

Closed
chawax opened this issue Jan 6, 2019 · 6 comments · Fixed by #10887
Closed

[gatsby-source-wordpress] Error in includedRoutes documentation ? #10871

chawax opened this issue Jan 6, 2019 · 6 comments · Fixed by #10887
Labels
good first issue Issue that doesn't require previous experience with Gatsby type: documentation An issue or pull request for improving or updating Gatsby's documentation

Comments

@chawax
Copy link

chawax commented Jan 6, 2019

Hi,

I try to use includedRoutes option of gatsby-source-wordpress plugin but could not make it work following the documentation here : https://www.gatsbyjs.org/packages/gatsby-source-wordpress/.

I added these lines in my gatsby-config.json file :

includedRoutes: [
    "/*/*/categories",
    "/*/*/posts",
    "/*/*/pages",
    "/*/*/tags",
    "/*/*/taxonomies",
    "/*/*/users",
],

But when I run gastby develop and enable verbose output I have such unexpected messages :

Route discovered : /wp/v2/posts
Excluded route: not whitelisted

I debugged the plugin and found that the following method in fetch.js file :

function checkRouteList(routePath, routeList) {
  return routeList.some(route => minimatch(routePath, route));
}

As you can see that this method uses the route path including base url (for example https://mywebsite.com/wp-json/v2/posts) and this doesn't match /*/*/posts.

I had to change gatsby-config.js this way to make it work :

includedRoutes: [
    "**/*/*/categories",
    "**/*/*/posts",
    "**/*/*/pages",
    "**/*/*/tags",
    "**/*/*/taxonomies",
    "**/*/*/users",
],

Is this an error in the documentation ? Or is there something I didn't understand ? (I am surprised I found nothing about this problem)

@sidharthachatterjee sidharthachatterjee added type: documentation An issue or pull request for improving or updating Gatsby's documentation good first issue Issue that doesn't require previous experience with Gatsby labels Jan 7, 2019
@sidharthachatterjee
Copy link
Contributor

Thank you for opening this @chawax 🙂

This definitely looks like an issue with the documentation. We would love to receive a pull request fixing this! Would you be interested?

The file that needs to be updated is at https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-wordpress/README.md

@gurpreet-hanjra
Copy link
Contributor

Hello, is it okay if I can take this issue?
Pull request can be viewed here - #10887

@ghost
Copy link

ghost commented Jan 8, 2019

I had no issues with the glob patterns of documentation. But I didn't exclude anything. Is this only relevant to routes of which some endpoints have been excluded?

Like:

includedRoutes: [
  '/*/*/posts',
],
excludedRoutes: [
  '/*/*/posts/1234',
],

@pieh
Copy link
Contributor

pieh commented Jan 8, 2019

@chawax I just checked and currently documented route globs works for me (TM), but my routes looks like /wp/v2/posts (without protocol + host like in your example), did you set protocol option in gatsby-config to https?

In either case there is no harm in adjusting docs to use ** (it still works in my tests) but maybe we could look into why discovered routes differ.

@cardiv
By default includedRoutes glob is just ** meaning we will try to grab everything - you can use it to narrow it down to only things you are interested in (especially when you have lots of plugins installed that add lot of 3rd party rest endpoints that would make fetching data just take a lot of time). Also important is that we don't hit individual post endpoints ('/*/*/posts/1234') - we grab all the data from listing ('/*/*/posts')

@chawax
Copy link
Author

chawax commented Jan 8, 2019

@pieh I set protocol to https. I tried with http too but behaviour is the same. As I said I was surprised no one encountered the problem before. So it may come from my environment.

I saw minimatch lib is used to check the route name. I have version 3.0.2 in node_modules. I don't know if it can be part of the problem, but I work on OSX. And my node version is 10.13.0.

@riddla
Copy link
Contributor

riddla commented Jan 25, 2019

@pieh, I'm using the plugin on a wordpress.com hosted site and I'm trying to include e.g. whitelist some routes.

getRoutePath strips the base url from the route and then it is checked via checkRouteList.

In my case it ends up checking for example /pages, so neither the new **/*/*/pages or the old /*/*/pages would do the trick.

The working configuration:

{
  resolve: `gatsby-source-wordpress`,
  options: {
    baseUrl: `stoffwindelberatungjuliarose.wordpress.com`,
    protocol: `https`,
    hostingWPCOM: true,
    useACF: false,
    auth: {
      // [...]
    },
    includedRoutes: ['/pages*', '/media*'],
  },
}

riddla pushed a commit to riddla/gatsby that referenced this issue Feb 15, 2019
This is picking up the discussion on the routes documentation within gatsbyjs#10871 and the according pull request gatsbyjs#10887.

It adds a test for a pattern that is working both on custom hosted WordPress API paths and on API paths from sites  that are hosted on wordpress.com.
DSchau pushed a commit that referenced this issue Feb 19, 2019
## Description

Adds a test for a route blog pattern that is working both on custom hosted WordPress API paths and on API paths from sites  that are hosted on wordpress.com.

## Related Issues

This is picking up the discussion on the routes documentation within #10871 and the according pull request #10887.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issue that doesn't require previous experience with Gatsby type: documentation An issue or pull request for improving or updating Gatsby's documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants