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

Include baseUrl in request object #35

Open
XuluWarrior opened this Issue Dec 28, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@XuluWarrior

XuluWarrior commented Dec 28, 2017

The endpoint for lambdas will have the stage as their baseUrl. However, currently, requests sent to the wrapped express app don't include this. This means that redirects will not work as expected.

e.g. a request to an express.static directory path without the trailing "/" attempts to redirect to the same path appended with "/". But when wrapped in serverless-http the redirect is wrong.

https://{host}/{stage}/staticdir => https://{host}/staticdir/

@XuluWarrior

This comment has been minimized.

XuluWarrior commented Dec 28, 2017

Similarly originalUrl should be set and equal to baseUrl + url

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Dec 28, 2017

Hmm, I think I understand. I run all of my serverless-http lambdas behind a Custom Domain that is at root - e.g. myapi.foo.com -- stage is never part of it. (In other words, I use an empty base path mapping).

I will do some testing to see if the baseUrl is correct when using a Custom Domain in order to make sure this works for both cases (with or without base path mapping)

@dougmoscrop dougmoscrop self-assigned this Dec 28, 2017

XuluWarrior pushed a commit to XuluWarrior/serverless-http that referenced this issue Dec 28, 2017

@XuluWarrior

This comment has been minimized.

XuluWarrior commented Dec 29, 2017

I've created a pull request with my suggested changes. It includes both my original thought of just making the baseUrl = /{stage} (now commented out) as well a possibly more robust solution.

I thought I would try using serverless-offline for debugging and I noted that the offline server doesn't include stage in the path. Nor does it provide a value for event.requestContext.path.

By comparing event.path with event.requestContext.path, I'm hoping that that will cover the cases of

  • default deployment
  • custom domains and
  • serverless-offline (which I guess is a special case of custom domains)
@XuluWarrior

This comment has been minimized.

XuluWarrior commented Jan 12, 2018

I set up a custom domain to test my changes and they work as expected. (Or at least as I would expect.)

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Jan 12, 2018

@lfreneda

This comment has been minimized.

lfreneda commented Feb 9, 2018

same issue, news?

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Mar 28, 2018

Sorry, the other PR does not fix it and breaks things for offline mode and I don't think works the way one would expect.

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Oct 26, 2018

I've finally been able to spend time on this.

I don't see how setting originalUrl or baseUr is making a difference - what do you expect the app to do?

Like res.redirect('/bar') doesn't add baseUrl using express as far as I can tell.

The application needs to be aware. It is non-trivial for applications to be "mounted" with some relative URL, especially if the document they return are using links in the body, such as a hypermedia API.

@XuluWarrior

This comment has been minimized.

XuluWarrior commented Oct 26, 2018

@dougmoscrop Thanks for looking into this.

I wrote up my particular use case in #42 (comment)

The key point is that express.static attempts to take into account mounting on relative URLs and uses baseURL or originalURL (I forget which now without looking at the code again) to do this.

@dougmoscrop

This comment has been minimized.

Owner

dougmoscrop commented Oct 26, 2018

OK, I will add these properties and hope that it doesn't interfere, but for an actual turnkey solution with the rest of the app routing infrastructure, especially cross-framework, I don't see how some userland configuration isn't necessary.

I was looking in to hacking on the router stuff in express to try to do something like this (it doesn't work of course)

// in serverless-http
module.exports = (app, opts) => (event) => 
  const handler = getHandler(app);
  const request = new Request();
  app.mountpath = getEffectiveMountPath(event)
  handler(...)
  app.mountpath = '/' // put it back for subsequent requests

Basically, "dynamically mount" the apps per request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment