-
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
Add endpoint to refresh external data #3054
Conversation
Deploy preview ready! Built with commit 64c6d2a |
Deploy preview ready! Built with commit 64c6d2a |
This would indeed be very useful — rather than having every plugin coming up with a way to refresh data, having a |
@KyleAMathews curious if this is something you would consider supporting before getting deeper into development. If not, could we expose some way to add a community-supported preview server extension (or if this is already supported, please let us know how)? |
That was never the idea — all source plugins should be idempotent i.e. Gatsby can call them whenever it wants. That's why I've been suggesting adding an extension to |
Exactly like this PR is doing :-) |
@PepperTeasdale I think a built-in preview "server" makes a ton of sense |
Awesome! I'll start by fleshing out this webhook to refresh the dev server, since its something we'd probably like in both dev and preview. Then, preview feature would be more focused on a similar webpack config, but without HMR and more efficient source maps, etc. |
c4685aa
to
213981d
Compare
@KyleAMathews 👀 please :) No longer a WIP. I would like to add some tests, but wasn't seeing where this kind of stuff was being tested in the project. If you have any ideas on that, please let me know. |
213981d
to
2841ada
Compare
The external source data can be refreshed by making a POST request to the `__refresh` endpoint. If the env var GATSBY_REFRESH_TOKEN is present there needs to be a matching `Authorization` header. This post request is wrapped for convenience with the command `gatsby refresh` which uses localhost:8000 by default, but allows for a host and port to be specified in the same way as `gatsby develop`.
2841ada
to
64c6d2a
Compare
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.
Great work! Covers all things we've been looking for in a preview server setup, right down to elegant handling of the refresh_token. And with a lot less code changes than I had imagined!
@KyleAMathews is there anything preventing this from getting merged? I guess this feature falls under advanced usage, but is more documentation needed? An example setup?
* If no GATSBY_REFRESH_TOKEN env var is available, then no Authorization header is required | ||
**/ | ||
app.post(`/__refresh`, (req, res) => { | ||
if (req.headers.authorization === process.env.GATSBY_REFRESH_TOKEN) { |
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.
nitpick or misunderstanding perhaps, but if process.env.GATSBY_REFRESH_TOKEN
is undefined, shouldn't refresh happen regardless of what is passed in res.headers.authorization
?
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 having a look! You point about the authorization headers makes sense. Also intend on adding appropriate documentation. Just wanted to handle any requested changes to the implementation details before writing it out (eg if the refresh token is defined as a command line argument when starting the server, rather than an env var).
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 think I'd personally vote for it to be an env var
I was thinking about it this weekend, and it might make sense to open this up as a hook for plugins, something like Nothing here would prevent future development of something like that. |
@@ -105,6 +105,25 @@ function buildLocalCommands(cli, isLocalSite) { | |||
handler: getCommandHandler(`develop`), | |||
}) | |||
|
|||
cli.command({ |
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.
Are you assuming that you'd have gatsby develop
running and then run this in another terminal tab? If so, this wouldn't work as Gatsby keeps the node data in memory — we need to add the ability to pass commands to the running gatsby develop process e.g. press "r" and a refresh would be triggered.
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 looking! This does work (in my local tests, at least), as it makes a post request to the port the server is running on, hitting an endpoint that calls sourceNodes()
.
If I understand correctly, you envision that the process running gatsby develop
is also listening to stdin
and updating based on input there, (eg: pressing "r" in the terminal window). I'm not familiar with any programs that do that, but I can totally give that a shot.
Even without the refresh
command, would you be open to the dev server accepting a webhook to reload the data? That is my use case and what some others commenting on the related issues are looking for.
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, pressing r
as in reload (kind of like how Jest CLI works?) could be nice for local development, but I agree with @PepperTeasdale that the main use case would be to use the webhook … since it's just a POST, building a trigger that fits the requirements on top of it would be trivial.
@KyleAMathews Just bumping to see if you would be interested in supporting the webhook portion of this PR. And if you would, what changes would be required for a merge? Thanks! |
@PepperTeasdale yeah! Let's ship it first w/o CLI command so we can test it w/o confusing people initially for what it's for. That gives y'all more flexibility for trying stuff early on. Later we can add the CLI command & documentation, etc. |
Dark deploy :-) |
Thanks @KyleAMathews ! Closing this and putting in the slimmed down PR at #3293 |
@PepperTeasdale, @KyleAMathews |
@wysher I need to make a PR to remove this code. It broke after a big fix went in. We ended up abandoning this path in our efforts to make a preview server. |
@PepperTeasdale - when you say you're going to submit a PR to remove this code, which code are you referring to? The code from #3293? I am just curious because we are about to set up a preview server and we were planning to use the refresh endpoint you added in that PR. Could you let me know if that will be removed soon then? It will change our course of action. |
@camsloanftc I do think that #3293 should be removed |
Although @Jrousell said they got it working last week, so perhaps I am wrong about this. |
I had it working before as well, with the exception of some Contentful data that seemed to be stuck in cache when attempting to delete content. I will try to set it up again to see what the status is. |
I was just testing out this feature again and had it working fairly well. I made a video of the process. I will summarize it here though: I added the environment variable: Then I setup a webhook in Contentful to access my local machine. I used ngrok to make a secure tunnel to do so. This is just for demonstration, usually this would point to a real server. Once I run Where I ran into trouble was archiving or deleting items. It does not seem to bust through what is stored in the If there is interest in seeing a video of this in action, I have it posted here. It seems to me like we may not need to remove this code, but figure out the issues with the cache. |
@camsloanftc very cool video! Looks like this TODO just needs done https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-contentful/src/gatsby-node.js#L73-L81 |
👋 @camsloanftc @KyleAMathews that can be easily done by https://www.contentful.com/developers/docs/references/content-delivery-api/#/reference/search-parameters/links-to-entry |
Fixes #2847 . The external source data can be refreshed by making a POST request to
the __refresh endpoint. If the env var GATSBY_REFRESH_TOKEN is present
there needs to be a matching Authorization header. This post request
is wrapped for convenience with the command gatsby refresh which uses
localhost:8000 by default, but allows for a host and port to be
specified in the same way as gatsby develop.