-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Allow Gatsby/Contentful to work offline #3977
Conversation
Deploy preview for gatsbygram ready! Built with commit 13fb510 |
I'll refactor this and push new logic down into |
Deploy preview for gatsbygram ready! Built with commit 97f3b1c |
@@ -29,9 +53,10 @@ module.exports = async ({ spaceId, accessToken, host, syncToken }) => { | |||
console.log( | |||
`Accessing your Contentful space failed. Perhaps you're offline or the spaceId/accessToken is incorrect.` | |||
) | |||
// TODO perhaps continue if there's cached data? That would let | |||
// someone develop a contentful site even if not connected to the internet. | |||
// For prod builds though always fail if we can't get the latest data. |
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.
I'm not sure what the optimal user workflow is here. I've opted for forcing the user to acknowledge a loss of connectivity rather than trying to auto serve cache if network is down.
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.
Opt-in require user to know about this feature in first place and if they don't have connectivity they can't actually look for solution for this online.
There are 2 ways to solve it:
- do automatic fallback and add console message that cached data is used because plugin couldn't connect with contentful api
- log that plugin can't connect and provide information how to enable this feature
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.
My bad, responded to your comment without reading your code change. I think this is better than other solution as this will cause less confusion that possibly stale data is used and requiring user action means user will acknowledge use of cached data.
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.
cool 👍
This is great! However there is scenario where I want to do something like this for wordpress plugin (one of my project has 400 images in media library and everytime I do change in gatsby-node I need redownload them, ugh) |
@pieh a persistent cache strategy seems like a useful addition, re-pulling 400 images must be annoying and inefficient. Shall we address that as a separate topic or as part of this PR?
|
I think that is out of the scope of this PR as it would involve changes in core, so that would be separate topic. Just figured it's always worth at least mentioning possible gotchas. |
This is really something that needs to be solved in core. I've been planning to tackle this for v3. Basically how it'd work is:
That being said — this is a pretty non-intrusive PR that seems safe to let in to let you and others start testing this. Let's just not document it and add a comment in the code saying that this functionality will be removed once core supports offline editing. Thoughts? |
@KyleAMathews yep, makes absolute sense to solve this holistically in core. I'll add the comment and update the PR. |
|
||
async function writeCacheFile(dir, result) { | ||
try { | ||
await fs.writeJson(`${dir}/${cacheFilename}`, result) |
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 should use path.join
.
@@ -63,6 +63,7 @@ exports.sourceNodes = async ( | |||
spaceId, | |||
accessToken, | |||
host, | |||
cacheDir: `${store.getState().program.directory}/.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.
As should this
@uttrasey It'd be great to get this merged if you have the chance to update it? In order to clean up the PR list I'll close hanging PRs like this over the next few days. But it's always better to merge a PR, so drop a comment here if you'd like to keep this open :) Or feel free to re-open this later. |
@Khaledgarbaya I think this just needs to merge master in / resolve conflicts and address Kyle's comments. Potentially I would also add to console output after |
This is continued by @Khaledgarbaya in #5869 , so I'm closing this as inactive. |
GATSBY_CONTENTFUL_OFFLINE
environment variable will make use of a cached fetch results from last successful comms with CMS. Useful if you want to develop/build without an internet connection.Pretty rudimentary but works.