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

add res.locals.use #1121

Closed
wants to merge 1 commit into from
Closed

add res.locals.use #1121

wants to merge 1 commit into from

Conversation

smagch
Copy link

@smagch smagch commented May 3, 2012

This is a experimental implementation of feature request issue 1120 which I opened.

I still don't quite understand connect/express ecosystem. As I see in issure 1000 app.local is implemented by expando rather than prototype inheritance.

I don't know about javascript performance, but it seems to me that mixining to every response as I implemented is quite inefficient.

@smagch
Copy link
Author

smagch commented May 3, 2012

Ah, I didn't know pull request automatically opens a issue...

@tj
Copy link
Member

tj commented May 3, 2012

nah it's not inefficient relative to anything else connect/express/your application is doing. The reason it is assign lazily like this is because it needs to "mixin" to itself since you can do res.locals({ obj: 'here' }). Even if we could change it to the prototype it would be a veryyyy small micro-optimization

@smagch
Copy link
Author

smagch commented May 4, 2012

Yeah, exactly. It would be very tiny since local.init() isn't doing heavy stuff. It seems api of locals is nice but it also causes a tiny shortcoming that it need to exposes "use" and "callbacks" field directly on local.

I'll add more test and comments when I have a time.

@tj
Copy link
Member

tj commented May 4, 2012

I added it already :D dd33ef2

@tj tj closed this May 4, 2012
@smagch
Copy link
Author

smagch commented May 5, 2012

Ah, I missed that commit . Thanks.

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

Successfully merging this pull request may close these issues.

2 participants