Subapps are not mounted

[winniehell]

When I access req.app.mountpath in the Router of a subapp, the behavior is different than from without streamline.

Without express-streamline I get

$ npm test
> curl localhost:3456/subapp/
/subapp
/subapp

but with streamline I get

$ npm test
> curl localhost:3456/subapp/
/
/subapp

Edit by @aseemk: diff between branches

[aseemk]

Thanks for the bug report and thorough example! Yes, I see, you're right.

I'm not sure off the top of my head what would cause this to not work, but I need to take a detailed look at Express 4 in general.

Express routers are valid middleware functions, according to this:

http://expressjs.com/4x/api.html#app.use

So the assumptions express-streamline makes should be valid. But in case they're not, I have a hunch that the issue is somewhere here:

https://github.com/aseemk/express-streamline/blob/0.5.0/index.js#L111-L118

If that's the case, one thing you could try as a workaround for now is to pass a second function to your app.use call, just a dummy middleware handler that does nothing.

Does that fix it for you? If so, that'd be a clue for fixing this issue at the root.

Thanks again for the report and thorough example! When I get to this, I'll make a test case from it.

[winniehell]

I just started using Express which makes it hard for me to follow your explanation. 👽
Can you help me understanding what you mean by

pass a second function to your app.use call, just a dummy middleware handler that does nothing

My current workaround is to manually set the mountpath... 😃

[winniehell]

I simplified the example apps a bit and made it (not) work for Express 3. Same issue there.

[aseemk]

Oh, nice — didn't realize even that simple workaround was possible. Great to hear. =)

And sweet, thanks for the repro on Express 3 too. That's helpful.

[winniehell]

I think I understand the problem now but I don't know how to fix it:

For

var subapp = express();
var app = express();
app.use('/subapp', subapp);

the condition at this point is true for subapp (because of this). Therefore subapp gets wrapped and app.use is called with the wrapper instead of subapp directly. This then causes app.use to think that subapp is no Express app because of this condition and setting the mountpath is skipped. A similar check is done for Express 2 and Express 3.

[winniehell]

How about adding the "Am I an Express app"-condition (lastArg.handle && lastArg.set) here?

[aseemk]

Oh wow, I missed that this was a pull request and not an issue. Thanks for submitting!

I don't have time right now to review and test it, but I hope to soon!

[aseemk]

Actually, I see that this is pretty straightforward. Nice work!

It generally LGTM if we can confirm that the tests pass across all versions of Express.

To that end, could you rebase this to the latest master so we know?

# add this repo as an upstream remote:
git remote add upstream https://github.com/aseemk/express-streamline.git

# on your branch, rebase to the current upstream master;
# you can optionally choose to squash some commits together:
git rebase --interactive upstream/master

# ensure that tests still pass, i.e. something didn't break:
npm test

# if at any point, you feel something broke, you can always abort via:
# git rebase --abort

# assuming everything is good, push the latest;
# the `+` is required since this has "rewritten" your branch's history;
# this also assumes your fork's remote is named `origin`:
git push origin +head:patch-1

If any of this is too complex or daunting for you, no worries at all. Just let me know and I'd be happy to test this myself.

Thank you again!

[aseemk]

Minor: let's move this helper function out of patch(), since it doesn't need to be re-defined for every verb.

[winniehell]

• done

[winniehell]

@aseemk Thank you for the review and all the helpful comments! All makes sense to me and I changed it.

For all my forks I actually have two remotes, so it was basically a git fetch --all && git rebase github/master 😉

I pushed the changes in two steps such that there are two builds confirming that the patch targets the issue:
https://travis-ci.org/aseemk/express-streamline/builds/51565117
https://travis-ci.org/aseemk/express-streamline/builds/51567097

P.S. I recently learned that you can convert issues to pull requests... maybe that explains where this pull request suddenly came from 😀 Unfortunately, you can't change the branch (name) of a pull request afterwards.

[aseemk]

Excellent work! Thank you for the thoroughness and attention to detail. =) Merging!

P.S. Ah, got it! Thanks for explaining. =D