Skip to content
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

Request.stream doesn't trigger('response') #335

Closed
astro opened this issue Jul 9, 2010 · 11 comments
Closed

Request.stream doesn't trigger('response') #335

astro opened this issue Jul 9, 2010 · 11 comments

Comments

@astro
Copy link

astro commented Jul 9, 2010

Implication: when I use stream() plugins such as Cookie don't work.

@aheckmann
Copy link
Contributor

It is currently being triggered? http://github.com/visionmedia/express/blob/master/lib/express/request.js#L251
How do you have it set up?

@astro
Copy link
Author

astro commented Jul 9, 2010

I'm just using stream() in combination with the Session plugin, requiring the Cookie plugin.

The problem with line 251 is that 'response' is now triggered on 'end'. The Cookie 'response' handler wants to add response headers (Set-Cookie:), so this needs to happen before sendHead().

@aheckmann
Copy link
Contributor

ooh yeah. yuck. I think we should hold off a bit on this until TJ is finished rewriting Express to use Connect next week. It would be best addressed there I think

@astro
Copy link
Author

astro commented Jul 9, 2010

@tj
Copy link
Member

tj commented Jul 11, 2010

Yeah I'm not accepting patches on this branch. Also re your tweet feel free to work on the "crappy view system" in the connect branch.

@astro
Copy link
Author

astro commented Jul 12, 2010

The master branch wasn't visibly abandoned to me. Now that I know connect I found it sufficient for my tasks.

The view system is crappy because it isn't asynchronous, node's most prominent code pattern.

@aheckmann
Copy link
Contributor

if you take a closer look you'll see that all views and partials are cached when running in production and are also configurable. this works well b/c it keeps the code simple and fast. we had truly async partials but the code got more complex for no real benefit. async in development mode doesn't matter.

@tj
Copy link
Member

tj commented Jul 13, 2010

yeah exactly. it is easy to argue that views should have async support, and in some cases I totally agree, however for rendering html I find the use case for this extremely low. Mu is no faster (infact slower) than haml.js for example.

Logic should be in the "route" handler, I dont like the idea of views hitting the database etc. When a practical need for async support exists then I will gladly support it, however at the moment I dont see the need for this. If you are generating a feed or something, its great, but then there is also not really a need to use the view system at all

@tj
Copy link
Member

tj commented Jul 13, 2010

blah lol github didnt post my reply. There is very little (practical) need for async views, I dont want that sort of logic in views, and Mu is no faster (infact slower) than haml.js. There is no reason you cant do async stuff before handing over to the view system, which IMO is the way it should be. If / when practical use cases appear I will happily support async template engines.

We have already been discussing this in IRC, and many of us agree that flushing the document HEAD early should be obtainable, but all of these async partial hacks etc are completely unnecessary

@aheckmann
Copy link
Contributor

i got the email tho

@tj
Copy link
Member

tj commented Jul 13, 2010

weird lol

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants