-
Notifications
You must be signed in to change notification settings - Fork 183
Add query string to type definition (gatsby-config.js) in order to handle i18n #193
Conversation
src/fetch.js
Outdated
const params = { _limit: queryLimit, ...api?.qs }; | ||
|
||
// Retrieve qs params if defined (or empty string instead) | ||
const qs = Object.keys(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't necessary with axios you can pass the query string as an object into the params property of the options
example:
const requestOptions = {
method: 'GET',
url: apiBase,
params: { _limit: 100, ...api?.qs },
headers: {}
}
await axios(requestOptions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, better like this... Fixed in commit below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thanks for submitting this PR so early after the i18n release.
I tested it locally and it's working fine. I just have one concern with it. It's about these examples in the readme:
// exemple fetching only english content
{
name: `collection-name`,
api: { qs: { _locale: 'en' } }
},
It seems like when we provide an entityDefinition object, the pluralize module is disabled. So I'm afraid users might copy this config and get a 404, because the plugin will not pluralize the collection name to find the endpoint.
So I would recommend either:
- Adding the endpoint property to the readme examples
- Only disabling the pluralize module if the endpoint key is provided. Not just if there's an entityDefinition object
README.md
Outdated
@@ -58,6 +69,23 @@ You can query Document nodes created from your Strapi API like the following: | |||
} | |||
``` | |||
|
|||
You can query Document nodes in chosen language | |||
> make sure to add `api.qs._locale` to your strapi conf in `gatsby-config.js` (see example above) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strapi conf is its own thing, so I'd recommend typing "configuration" entirely
Also, I don't think this needs to be a quote (>
), you can add this as text directly
And I would say "You can query Document nodes in a chosen language"
@remidej you were right, I've been missing pluralization if no endpoint provided, this is fixed now. (So I've not updated the example in the README, will work just fine now). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix, it's working well now
Hi guys ! Thank you for your work :D Do you have an idea for when it will be usable and published in npm ? :) Thank you ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi everybody ! :) Do you know when the feature will be available on npm ? Thank you |
Hello @juliensl We just published an alpha release with this feature: https://www.npmjs.com/package/gatsby-source-strapi/v/1.0.0-alpha.2 Just know that there's a small mistake in the readme example that I'll fix tomorrow:
|
@alexandrebodin there we go, I applied your idea that was indeed the best way to handle this problem.
I've updated README accordingly.
I've also added
queryLimit
param to this new behavior. It can now be overriden individually for each type.