update: fix async-app app chaining when mounting sub app#45
Draft
micaelbergeron wants to merge 2 commits into
Draft
update: fix async-app app chaining when mounting sub app#45micaelbergeron wants to merge 2 commits into
micaelbergeron wants to merge 2 commits into
Conversation
micaelbergeron
commented
Feb 7, 2024
Comment on lines
+117
to
+162
| if (context.method === 'use') { | ||
| return args; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This might be a little bit too eager — one other solution would be to make isMiddleware return false whenever we send a App instance.
Collaborator
There was a problem hiding this comment.
I think this is fine!
micaelbergeron
force-pushed
the
fix/async-app-mount
branch
from
cd0a7c7 to
7df9e5f
Compare
When mounting a sub app with `app.use(path, subApp)`, express does a check to see if an app was sent through, or if it was a middleware. In async-app, we are converting all MiddlewareArguments which confuses express to think that the mounted app isn't an sub application, but a simple middleware. Thus, express doesn't fire the `mount` event, which in turns skips a bunch of express routines: - sets the `mountpath` on the subApp - copy some settings from the parent app (for instance "trust proxy") With this change, we can now use `req.app.path` to get the full route the request was matched for, and `req.ip` will be properly propagated through.
micaelbergeron
force-pushed
the
fix/async-app-mount
branch
from
c513aef to
64ab37a
Compare
nicolapalavecino
approved these changes
Feb 27, 2024
Collaborator
nicolapalavecino
left a comment
nicolapalavecino
left a comment
There was a problem hiding this comment.
Amazing finding!
|
|
||
| Scenario: mounted apps (nest level 1) | ||
| Given app setting "trust proxy" is enabled | ||
| Given the request header X-Forwarded-For is "1.1.1.1" |
Collaborator
There was a problem hiding this comment.
NITPICK:
Suggested change
| Given the request header X-Forwarded-For is "1.1.1.1" | |
| And the request header X-Forwarded-For is "1.1.1.1" |
|
|
||
| Scenario: mounted apps (nest level 2) | ||
| Given app setting "trust proxy" is enabled | ||
| Given the request header X-Forwarded-For is "10.0.1.3" |
Collaborator
There was a problem hiding this comment.
NITPICK:
Suggested change
| Given the request header X-Forwarded-For is "10.0.1.3" | |
| And the request header X-Forwarded-For is "10.0.1.3" |
Comment on lines
+117
to
+162
| if (context.method === 'use') { | ||
| return args; | ||
| } |
Collaborator
There was a problem hiding this comment.
I think this is fine!
| onTearDown, | ||
| }) => { | ||
| // === App setup === // | ||
| Given('app setting "(.*)" is enabled', (setting) => { |
Collaborator
There was a problem hiding this comment.
NITPICK: I would refer this step for enabling a setting for a specific app
Suggested change
| Given('app setting "(.*)" is enabled', (setting) => { | |
| Given('the (advanced|basic) app setting "(.*)" is enabled', (setting) => { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When mounting a sub app with
app.use(path, subApp), express does a check to see if an app was sent through, or if it was a middleware.In async-app, we are converting all MiddlewareArguments which confuses express to think that the mounted app isn't an sub application, but a simple middleware.
Thus, express doesn't fire the
mountevent, which in turns skips a bunch of express routines:mountpathon the subAppWith this change, we can now use
req.app.pathto get the full route the request was matched for, andreq.ipwill be properly propagated through.