-
Notifications
You must be signed in to change notification settings - Fork 183
Conversation
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
e2bc8b3
to
0ff61e2
Compare
I have a working solution here (https://github.com/relate-app/gatsby-source-strapi/) that solves all the checkpoints except maybe cloud - not sure what that includes. Please have a look, it uses strapi graphql rather than rest which has it's limitation, i.e. not exposing types properly which is available through the schema introspection on the graphql side of things. Hope it helps! // T |
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
1da6df0
to
fb02bbc
Compare
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Only fetch data after build
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Extract markdown files
Signed-off-by: soupette <cyril@strapi.io>
Refactoring
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Content Synce
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Add support for user content type and file content type
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Update readme
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
Fix readme
src/gatsby-node.js
Outdated
* | ||
* See: https://www.gatsbyjs.com/docs/node-apis/ | ||
*/ | ||
// You can delete this file if you're not using it |
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.
Probably good to remove this line :)
src/gatsby-node.js
Outdated
const allResults = await Promise.all( | ||
endpoints.map(({ kind, ...config }) => { | ||
if (kind === 'singleType') { | ||
return fetchEntity(config, ctx); | ||
} | ||
|
||
return fetchEntities(config, ctx); | ||
}) | ||
); | ||
|
||
let newOrExistingEntries; | ||
|
||
// Fetch only the updated data between run | ||
if (lastFetched) { | ||
// Add the updatedAt filter | ||
const deltaEndpoints = endpoints.map((endpoint) => { | ||
return { | ||
...endpoint, | ||
queryParams: { | ||
...endpoint.queryParams, | ||
// TODO | ||
filters: { | ||
updatedAt: { $gt: lastFetched }, | ||
}, | ||
}, | ||
}; | ||
}); | ||
|
||
newOrExistingEntries = await Promise.all( | ||
deltaEndpoints.map(({ kind, ...config }) => { | ||
if (kind === 'singleType') { | ||
return fetchEntity(config, ctx); | ||
} | ||
|
||
return fetchEntities(config, ctx); | ||
}) | ||
); | ||
} | ||
|
||
const data = newOrExistingEntries || allResults; |
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.
Does this still fetch all nodes on each run? Shouldn't there be a conditional to not fetch allResults
if there's a lastFetched
key?
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.
Well we need to fetch all the results in order to know which content has been deleted.
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.
Does allResults mean all node data in Strapi? If so, needing to fetch that to determine what was deleted means the plugin doesn't really have incremental builds support.
Unless I'm misunderstanding, this code fetches all data on cold builds then on subsequent cached builds it again fetches all data and then additionally double fetches anything that changed since the first build. Is that right or am I misunderstanding?
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.
On subsequent cached build it uses the delta fetched data to create/update nodes. The first call is only used to know which nodes should be deleted.
So on cold build we use all the data and on then we only use the other fetches to have smaller rebuilds
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.
Nice 👍 good to hear that 😄 Does that happen inside fetchEntities? I'm having trouble finding it
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.
Not it happens with the filer updatedAt
that is set with the lastFetched
constant retrieved from the cache
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 👍 I get that part but I'm not sure how allResults
on line 71 doesn't refetch all data on each build. On cold builds that's what pulls in all initial nodes right? I'm not following how it doesn't re-fetch all nodes again on warm builds.
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.
allResults
are always fetched it is used to know which nodes we need to delete, however on subsequent build if we have already requested the API the data we use is the result of the delta fetches from L.85.
Is that an issue? Should I make an update?
If you check L.96 you'll see that we use the delta results if it exists otherwise the full results.
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.
Ah, yeah the problem is in which data is fetched, not in which is used. Data fetching is the slow part (especially for large sites) so if we need to refetch all results on every build then the delta isn't really helping. Ideally there would be a way to fetch deletes as part of the delta too. Then on warm builds we would skip fetching all results and just fetch the delta.
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.
Would it be interesting to add this behaviour only when GATSBY_IS_PREVIEW
is true or should we always do it?
src/gatsby-node.js
Outdated
...endpoint, | ||
queryParams: { | ||
...endpoint.queryParams, | ||
// TODO |
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.
is this still a todo?
|
||
const nodeType = makeParentNodeName(ctx.schemas, uid); | ||
|
||
const mainEntryNode = nodes.find((n) => { |
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.
Is it possible to edit a block outside of a page in Strapi? In Contentful we create node manifests for all content nodes (even if they're not top level) because it means you can press "Open Preview" from within the edit view of a nested block.
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.
No it's not possible, only primary nodes, the one that create pages, regarding linked nodes the manifest will also be created for them as we fetch them
Signed-off-by: soupette <cyril@strapi.io>
Signed-off-by: soupette <cyril@strapi.io>
- [Image optimisation](#image-optimisation) | ||
- [Rich text field](#rich-text-field) | ||
- [Components](#components) | ||
- [Dynamic zones](#dynamic-zones) |
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.
Can you add a localizations section and accompanying documentation?
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.
cc/ @remidej @markkaylor
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.
Would it be okay to add this in a future release?
Because I think it will be easier once we'll have updated the Gatsby corporate starter which uses i18n
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.
That'd be a cool starter to have! I've got alpha 4 working with my test site and it's working well except for a few localization bits. Doing those in a discrete release could be a nice milestone
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.
May I chime in here? I'm just a regular developer watching the PR and tries it in development. While a starter is great to reduce startup time and complexity it's IMHO a nice to have while undocumented features makes the lib really hard to use until you release a new starter. I don't want to sound demanding here, you do a great job on this lib, I just wanted to give some user input if you find it helpful. 💚
Init plugin migration to Strapi v4:
Bug fixes:
gatsby build
the images in strapiArticle.content doesn't reference a local file #114