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

Memoize .helpers #306

Merged
merged 1 commit into from
Oct 12, 2012
Merged

Memoize .helpers #306

merged 1 commit into from
Oct 12, 2012

Conversation

dqminh
Copy link
Contributor

@dqminh dqminh commented Oct 12, 2012

In one of my apps, I have some specs that expect .helpers to be consistent for the same instance of presenter. However, it seems that Draper instantiates new instance of HelpersWrapper everytime helpers is accessed

This does not create new instance of HelpersWrapper every time ".helpers" is accessed

this does not instantiate new instance of HelpersWrapper
every time "#helpers" is accessed
@steveklabnik
Copy link
Member

So, this can be dangerous. The reason is that the context can change, like when you enter into a do block in a helper.

I guess that we are memozing the wrapper, and not Rails' context. Hm.

@haines
Copy link
Contributor

haines commented Oct 12, 2012

Memoizing the wrapper will memoize the context because we pass it in and store it in an instance variable in the wrapper.

@steveklabnik
Copy link
Member

Hmmm yeah. So I'm not sure this won't break stuff. That integration suite and app would be useful now...

@dqminh
Copy link
Contributor Author

dqminh commented Oct 12, 2012

Hmm, I thought that draper is also performing memoization of the view context with Thread.current. Is it susceptible to view_context change as well ?

@steveklabnik
Copy link
Member

Wellllllllll maybe. I'm not sure. See #124 for the weirdness.

I'm actually planning on re-writing the entire view context code later today. I had some discussions with @wycats about it, and we can do much better.

I'm gonna merge this for now, and we'll see if it breaks things.

steveklabnik added a commit that referenced this pull request Oct 12, 2012
@steveklabnik steveklabnik merged commit cb3878a into drapergem:master Oct 12, 2012
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.

3 participants