initial implementation for configureServer #4903
Conversation
|
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/8kgx6JWaoHBF9BgguRi9HtfNQf53 |
gautamsi
changed the title
gautamsi
changed the title
gautamsi
force-pushed
the
new-interface-trust-proxy
branch
from
adf4a03 to
d6def0e
Compare
|
@MurzNN has tested this and found that the value of |
| @@ -52,5 +52,10 @@ export default auth.withAuth( | |||
| // // store: redisSessionStore({ client: redis.createClient() }), | |||
| // secret: sessionSecret, | |||
| // }), | |||
| server: { | |||
| configureServer(app) { | |||
| app.set('trust proxy', true); | |||
There was a problem hiding this comment.
Setting trust proxy in this place give no effect seems because express is initialized earlier default values for serve Admin UI? Or maybe this is not enough to make session cookies working.
For investigate this we can enable verbose logging of express via starting DEBUG=express:* yarn dev and I see that this code is initialized later, that express startup. Even if I place console.log('configuring trust proxy'); in first line of examples-next/basic/keystone.ts file - it will be executed after initializing of express, here is output:
server: {
configureServer(app) {
console.log('CONFIGURING TRUST PROXY');
app.set('trust proxy', true);
}
The output is:
$ DEBUG=express:* yarn dev
yarn run v1.22.10
$ keystone-next dev
🤞 Starting Keystone
express:application set "x-powered-by" to true +0ms
express:application set "etag" to 'weak' +1ms
express:application set "etag fn" to [Function: generateETag] +1ms
express:application set "env" to 'development' +1ms
express:application set "query parser" to 'extended' +0ms
express:application set "query parser fn" to [Function: parseExtendedQueryString] +1ms
express:application set "subdomain offset" to 2 +0ms
express:application set "trust proxy" to false +0ms
express:application set "trust proxy fn" to [Function: trustNone] +0ms
express:application booting in development mode +0ms
express:application set "view" to [Function: View] +0ms
express:application set "views" to '/srv/korepov/domains/ks-t1.k.dev.digiterra.pro/src/keystone/examples-next/basic/views' +0ms
express:application set "jsonp callback name" to 'callback' +1ms
CONFIGURING TRUST PROXY
express:router use '/' query +3s
express:router:layer new '/' +1ms
express:router use '/' expressInit +1ms
express:router:layer new '/' +0ms
express:router use '/__keystone_dev_status' <anonymous> +0ms
express:router:layer new '/__keystone_dev_status' +0ms
express:router use '/' <anonymous> +1ms
express:router:layer new '/' +0ms
⭐️ Dev Server Ready on http://localhost:3000
✨ Generating graphQL schema
✨ Connecting to the database
(node:3661400) DeprecationWarning: Listening to events on the Db class has been deprecated and will be removed in the next major version.
✨ Generating Admin UI code
✨ Creating server
express:application set "x-powered-by" to true +1s
express:application set "etag" to 'weak' +0ms
express:application set "etag fn" to [Function: generateETag] +1ms
express:application set "env" to 'development' +0ms
express:application set "query parser" to 'extended' +0ms
express:application set "query parser fn" to [Function: parseExtendedQueryString] +0ms
express:application set "subdomain offset" to 2 +0ms
express:application set "trust proxy" to false +1ms
express:application set "trust proxy fn" to [Function: trustNone] +0ms
express:application booting in development mode +0ms
express:application set "view" to [Function: View] +0ms
express:application set "views" to '/srv/korepov/domains/ks-t1.k.dev.digiterra.pro/src/keystone/examples-next/basic/views' +1ms
express:application set "jsonp callback name" to 'callback' +0ms
✨ Preparing GraphQL Server
express:router use '/' query +58ms
There was a problem hiding this comment.
And even more - this code seems isn't even called, because adding:
server: {
configureServer(app) {
console.log('CONFIGURING TRUST PROXY');
app.set('trust proxy', true);
},
},gives no output of this line.
There was a problem hiding this comment.
it may only work if you are using this branch
|
Hey @gautamsi sorry it's been so long, I wanted to get back to you with some feedback on this PR I really like the API, and I see the importance of having this implemented. You've also designed it in a way that's consistent with similar config options which is great 👍 However, not too long ago we technically supported generating Keystone (both the Admin UI and the GraphQL API) as a Next.js app that could be deployed entirely to a Serverless environment like Vercel I'm really keen to get back to that, which this moves us ever so slightly further away from. Not saying that's a reason to block it, just that how the config options are set up really needs to be thought about comprehensively so they are clear for both use-cases. The answer is probably that this config option can only coexist with another specifying that the server target is We also need to carefully consider the lifecycle here -- is this configuration for the just-created Per my comment to @timleslie on #4912 I think we need to do some big picture config design, so we can confidently continue adding these features without accidentally conflicting with other planned changes; sorry that's delaying this, it's high on our list to get done but we're also putting most of our focus right now into documenting and cleaning up what's there as a starting point. Until then this is on the radar but unlikely to get merged, until we've really thought it through and are confident we're not painting ourselves into a corner. |
|
Further to that previous comment, the other thing to consider (given the lifecycle of setting up an express app is so delicate) is that maybe we should be exposing more server config for things like Again, that's not a straight-forward "yes" for me, just wanted to highlight that we really need to do some big picture config design here around the various use cases. |
|
@JedWatson thanks for clarifying the point. I had some chat with @timleslie on this. I agree that the new config scheme must be well thought per the future plan. I am also interested in serverless. That said, you are also right that this kind of request will be more from community. We can work on creating layers which can have certain aspect, like:
For now I am thinking of creating |
|
@JedWatson @timleslie should we use this as experimental feature like #5033 (comment) |
|
It looks like there hasn't been any activity here in over 6 months. Sorry about that! We've flagged this issue for special attention. It wil be manually reviewed by maintainers, not automatically closed. If you have any additional information please leave us a comment. It really helps! Thank you for you contribution. :) |
|
closing in favor of #6467 |
closes #4902
@JedWatson @timleslie I am not sure if you want to call this function name differently (
configureServerorconfigureExpressas in v5). initial try at this, hopefully this fixes the issue for trusting the proxy as well as enabling other scenarios like increasing payload size for graphql. I will add changeset if needed also this is very small change, so go ahead and fix this if you want to merge by changing few details.This is needed to enable support for custom express server config. I need it to publish new fields based on TinyMCE and CKEditor5. I will have to use the
configureServermethod and as additional step to enable the field but we can use it until you guys figure out a way to let Fields access express instance and configure static path there.