Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

tests for req/res proto restoration #1114

Closed
tj opened this Issue Apr 26, 2012 · 12 comments

Comments

Projects
None yet
5 participants
Owner

tj commented Apr 26, 2012

when mounting

Contributor

defunctzombie commented Nov 20, 2013

@visionmedia could you clarify this a bit more? Is this still something you are interested in?

Owner

tj commented Nov 20, 2013

we have some funky code to replace the protos so that subapp proto manipulation doesn't mess with parent apps, it should be working fine but we don't have any test-cases

Contributor

defunctzombie commented Jan 25, 2014

I think the goal should be to remove app.use(app) behavior in favor of users creating routers. If you want to juggle multiple apps (for whatever reason) then there can be a module that does that effectively.

Member

rlidwka commented Jan 29, 2014

I think the goal should be to remove app.use(app) behavior

By doing this you split one thing (app or middleware) into two things (app + middleware) with different interfaces (otherwise app.use would be possible). So developer would have to remember that, would have to know the differences, etc, etc.

It's a wrong way.

Member

jonathanong commented Jan 29, 2014

I think the goal should be to remove app.use(app) behavior in favor of users creating routers. If you want to juggle multiple apps (for whatever reason) then there can be a module that does that effectively.

not totally sure what this means. app.use(app) should still work since app is middleware. only thing that should really change is inheritance imo, but it wouldn't be a problem if people did app.use(router()) instead of new apps.

By doing this you split one thing (app or middleware) into two things (app + middleware) with different interfaces (otherwise app.use would be possible).

not totally sure what this means either. apps are middleware, but with this proto stuff. people should be able to use the router part of apps without all this proto stuff. they all have the same (req, res, next) signature.

Contributor

defunctzombie commented Jan 29, 2014

@jonathanong So app.use(another_app) would simply delegate to the other app as if it were middleware? What about the proto patches the apps do? What inheritance do we currently have? IIRC we don't have any and are not adding any. I personally view the "app" as a singleton and if you want separate files or routes then Router is the new documented approach. In my mind using app.use(another_app) was always a hack because the Router was not as full featured as it should have been.

@rlidwka I don't quite follow what you mean here. The distinction is that an app (to me) represents your single express application. Things like app.use(...) are now just proxies to the default Router you get with your app instance. If you want to create what use to be called "subapps" then you would create Routers now since they are fully features. The only thing remaining is exposing the View layer as middleware. This would allow folks to organize their subapp views within the subapp and use within the Router.

When you have app.use(another_app), we now need to think about how locals propagate and settings. If you make it so the user ever creates one app and everything else is self contained Routers then this is not an issue. Yes, calling something a "subapp" seems nice but we can certainly have documentation that just describes subapps in terms of Routers.

What I propose is some example code for each use case showing the pros/cons. I think we could debate this with words for a while but if we actually look at the issues that have come up with this and look at how it is implemented in apps in practice we will get a better idea of the pain points and what to include/exclude and document.

Member

jonathanong commented Jan 29, 2014

So app.use(another_app) would simply delegate to the other app as if it were middleware?

yeah. isn't that what it does now? it would also replace the proto, so your settings will be blank. i'm not sure if the original proto gets restored as well - it should.

this is the only inheritance i can find right now: https://github.com/visionmedia/express/blob/master/lib/application.js#L56

I personally view the "app" as a singleton and if you want separate files or routes then Router is the new documented approach. In my mind using app.use(another_app) was always a hack because the Router was not as full featured as it should have been.

totally agree!

Contributor

defunctzombie commented Jan 30, 2014

Kinda, but I think it also does some prototype setting stuff like you found. Also creates this "mount point" property. I believe the original proto does get restored.

https://github.com/visionmedia/express/blob/master/lib/application.js#L113

The settings get carried over cause of the proto replacement. So app.get(setting) works in the subapp currently. I think the only thing we would want to get rid of is the on mount stuff and the path stuff since you would not need to query that nor would it be of any use. The mounted app just gets treated like middleware and this happens with the __proto__ replacement.

My only argument for getting rid of app.use(child) is that anything you set in the child on your app there won't actually be used. And the same is true for view engines, etc (I think). We have had issues opened about this. With no ability to mount an app, it makes it clear that you can only use Routers (which don't have settings or other app level things).

Contributor

defunctzombie commented Jan 30, 2014

@reqshark please stop. You are off topic and spamming.

Contributor

reqshark commented Jan 30, 2014

ok

Contributor

reqshark commented Jan 30, 2014

@defunctzombie compliments on the block, G

Member

jonathanong commented Feb 23, 2014

closing because we're removing this proto restoration stuff.

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