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 ability to refetch partial from server to render within session context #44

Closed
chrismccord opened this issue May 27, 2013 · 27 comments

Comments

@chrismccord
Copy link
Owner

The ability to refetch partials from the server via ajax to render within the requesting users session context has been added in 0.2.0. This solves the issues of syncing partials across different users when the partial requires the session's context (ie. current_user).

Ex:
View: Add refetch: true to sync calls, and place partial file in a 'refetch'
subdirectory in the model's sync view folder:

The partial file would be located in app/views/sync/todos/refetch/_list_row.html.erb

<% @project.todos.ordered.each do |todo| %>
  <%= sync partial: 'list_row', resource: todo, refetch: true %>
<% end %>
<%= sync_new partial: 'list_row', resource: Todo.new, scope: @project, refetch: true %>

Notes
While this approach works very well for the cases it's needed, syncing without refetching should be used unless refetching is absolutely necessary for performance reasons. For example,

A sync update request is triggered on the server for a 'regular' sync'd partial with 100 listening clients:

  • number of http requests 1
  • number of renders 1, pushed out to all 100 clients via pubsub server.

A sync update request is triggered on the server for a 'refetch' sync'd partial with 100 listening clients:

  • number of http requests 100
  • number of renders 100, rendering each request in clients session context.

I'm looping in @aaronjensen, @anazar, @waymondo, @ericboehs, @oneiros, since they have sync code in the wild and this feature solves a few issues some have raised. Let me know how it goes!

@anazar
Copy link

anazar commented May 27, 2013

@chrismccord - Thanks so much for this Chris! I tried it out and it worked perfectly. I'll let you know once I work with this more if I have any feedback.

@cj
Copy link
Contributor

cj commented May 27, 2013

@chrismccord thank you for this! Quick question, is there a reason you've required it to be in a sub folder refetch?

@chrismccord
Copy link
Owner Author

@cj The reason a 'refetch' subfolder is required is because when a sync_update or sync_new is triggered from the controller, say for a Todo model, all the partials in app/views/sync/todos are renderered to string and pubsub'd out to listeners for that resource. Since the refetch: true partials must be rendered within a different context, they must be dinstinctly separated as to not be rendered automatically and published out with the 'regular' sync renders.

@crcastle
Copy link

I had to add get 'sync/refetch', controller: 'sync', action: 'refetch' to my app's config/routes.rb after updating the Sync gem. Without that my browser would get a 404 looking for sync/refetch.json route. Is that the correct setup? If so, should this be added to the install generator?

But otherwise this is great! I was struggling trying to figure out what to do with partials that needed to use current_user.

@chrismccord
Copy link
Owner Author

@crcastle Glad to hear it's working for you. Sync should add the routes to your application (https://github.com/chrismccord/sync/blob/master/config/routes.rb) and works without having to manually add the routes yourself. What version of rails are you running? I have a Rails 4 app up and running without having to mirror the routes, but I don't see why Rails 3 would have this issue. It it possible you needed to restart your webserver after updating the gem to pick up the engine's routes?

@waymondo
Copy link
Contributor

@chrismccord you the man! eager to try this out.

@aaronjensen
Copy link
Contributor

@chrismccord are you going to make it so that the default rendering context is not the current user's context now? Along with this change that would be great.

I would (and will) endeavor to never use this feature. I try and not do any user specific view rendering in my projects if I can avoid it, rather relying on client side code to modify the view for the individual user. This makes caching easier and it makes the multicast sync work.

@chrismccord
Copy link
Owner Author

@aaronjensen Yes I will be changing the default rendering context, and I agree with you in part. Refetching is to only be used when absolutely necessary, but there are plenty of valid scenarios/requirements where refetching would work extremely well. Even with the performance/caching hit, you still come out way ahead vs constant polling. If we consider Basecamp's approach of using heavily shared cached fragments decorated with css/js, they still poll constantly every three seconds for realtime updates. Given that, refetch: true more or less gives you on demand polling, where the partial only polls a single time when changes occur.

@aaronjensen
Copy link
Contributor

Yeah, I think the combination will work great. That is, sync to handle updates to the view but decorating w/ css/js to handle user specific rendering.

@ericboehs
Copy link
Contributor

This is handy. I may/should be using this in the coming weeks as I start to work more on projects with sync. Thanks for keeping me in the loop.

@crcastle
Copy link

@chrismccord, strange. I'm not sure why the engine's route (GET sync/refetch.json) are not getting picked up. I used your sync_example repo as my base but have upgraded to rails 4.0.0.rc1 and changed several other things. I'm using pow as my web server, but I also tried rails s just to make sure it wasn't an issue with restarting the server.

I'll see if I can figure out what's going on, but it's working with the mirrored route -- just not very clean.

@chrismccord
Copy link
Owner Author

@crcastle I was able to recreate this. I had my gem pointing to sync's github master branch. For some reason, the gem itself excludes the config directory when installing, thus leaving out the routes file. I'm trying to fix this now.

@chrismccord
Copy link
Owner Author

So if you point your gem to git: '[email protected]:chrismccord/sync.git', things work, but when I build the gem config/ is left out.

@chrismccord
Copy link
Owner Author

Doh. I forgot to include config as part of the gem build files. Please update to 0.2.2 where this is resolved. Thanks!

@oneiros
Copy link
Contributor

oneiros commented May 29, 2013

I just wanted to chime in and say thanks. This looks extremely useful.

I even think I needed something like this at some point but then found a workaround. Now I only need to remember where that was 😄

However, keep up the good work!

@waymondo
Copy link
Contributor

Just gave this a shot and upon the /sync/refetch.json call on the client side, I run into this view rendering error =>

ActionView::MissingTemplate - Missing partial sync/memo_replies/refetch/memo_reply with {:locale=>[:en], :formats=>[:json], :handlers=>[:erb, :builder, :arb, :slim, :coffee]}.

The partial is a slim partial and the complete path name is /app/views/sync/memo_replies/refetch/_memo_reply.html.slim. I found that if I patched ActionView in an initializer like the following, I got it to fallback to render the html template in the json call =>

module ActionView
  class LookupContext
    module ViewPaths
    protected

      def formats=(values)
        if values
          values.concat(default_formats) if values.delete "*/*"
          values << :html if values == [:js] || values == [:json]
        end
        super(values)
      end

    end
  end
end

I'm assuming there's a better way to do this though, any ideas?

@chrismccord
Copy link
Owner Author

What's the ActionView patch actually doing? Do you know if this is slim specific or do you receive the same error if the view was an erb templtae? Is the slim handler not being found since the request is of type json? (though it's listed in the error message as a handler). Once I know more I should be able to dig deeper. There's no reason at the moment the request type needs to be json, since it's just returning html, but I made it json for the eventual plan to allow batched array of refetched partials to be requested in a single http request; however that's not on the immediately roadmap so we may be able to fix this by making the request type/response html.

@chrismccord
Copy link
Owner Author

The odd thing is https://github.com/chrismccord/sync/blob/master/lib/sync/partial.rb#L20-L22 specifies that the formats should include :html, so I'm not sure why this isn't making it over in your case.

@waymondo
Copy link
Contributor

I'm just monkey patching this method and adding || values == [:json] so it falls back to HTML if a JSON template isn't found =>

http://apidock.com/rails/v3.2.13/ActionView/LookupContext/formats%3D

I did just test this with a dummy static ERB template at the same path ({}/_memo_reply.html.erb) and the patch removed and it worked so it must be happening in the :slim handler. Now that I'm thinking about it though, I think HTML might make more sense as as the default format for refetches_controller, since JSON traditionally represents data and not markup.

@MhdSyrwan
Copy link
Contributor

@waymondo I have the same issue, after debugging & searching i found that it's an issue in rails 3 rails/rails#4841
The partial always falls back to the response format (which is json here)

There's some solutions to do that like using this method

def with_format(format, &block)
    old_formats = formats
    self.formats = [format]
    block.call
    self.formats = old_formats
    nil
end

--from http://stackoverflow.com/questions/339130/how-do-i-render-a-partial-of-a-different-format-in-rails

we can solve it from the gem like:
in the file https://github.com/chrismccord/sync/blob/master/app/controllers/sync/refetches_controller.rb#L11

  def show
    partial_string = ""
    with_format :html do
      partial_string = @partial.render_to_string
    end

    render :json => {
      html: partial_string
    }
  end

@chrismccord should we solve the problem in the gem or just add a note for rails3 developers ?

@chrismccord
Copy link
Owner Author

@MhdSyrwan I wouldn't have a problem including your solution in the gem since it isn't monkey patching anything, we just need to double check with_format also plays nice with Rails 4. If you want to package your efforts as a pull request I would be glad to merge them in :). Please add a private RefetchesController method like:

  def show
    render :json => {
      html: with_html_format(partial_string)
    }
  end

Also if the with_format block returns it's last evaluated expression, we could simply make it:

  def show
    render :json => {
      html: with_format(:html){ @partial.render_to_string }
    }
  end

Thanks!

@cdeyoung
Copy link

cdeyoung commented Sep 2, 2013

I just recently started using Sync and have run into a situation that I thought refetch: might help with, but it doesn't seem to work, either because I'm doing it wrong or because I'm not understanding the purpose of refetch: correctly.

What I'd like to do is report the last user to post something by comparing the User object of the last post to the current_user. If they are the same, I'd display something like, "Your post is the most recent," to the user. If they are different, I'd post "Someone has posted after you."

What is happening is that Sync compares the owner of the last post to the value of the session's current_user, sees they match, and syncs the positive message to all users without comparing the value of current_user in each individual user's browser. Is there a way to use refetch: or some other aspect of Sync to force a comparison in such a way that only one user would receive the positive message and the rest receive the negative one?

I have created the following test code while attempting to get this to work:

Here is code from my sync/posts/refetch/_notify.html.erb file:
<p>:Last post by: <%= @last_post.user_id %> - You are: <%= current_user.id %></p>

Here is the code that calls the sync partial:
<%= sync partial: "notify", resource: @post, refetch: true %>

In the controller, I'm calling:
sync_update @post

What I'd like to get for the owner of the last post (who has a User.id of 1) is:
"Last post by: 1 - You are 1"
and I'd like to get this for my other user (who has a User.id of 2):
"Last post by: 1 - You are 2"

Instead, I'm getting "Last post by: 1 - You are 1" for both users.

I tried structuring my code like the example Chris gave at the top (using sync_new), but that returns the same results. Any help with this would be greatly appreciated.

@chrismccord
Copy link
Owner Author

@cdeyoung , when performing refetches, Sync renders the partial in a Sync::RefetchesController, allowing access to session based methods ie current_user, but it has no context of the controller action that you issued sync_update @post in, from your example.

This should get you up and running:

  1. Ensure you have your partial saved in app/views/sync/posts/refetches/_notify.html.erb
  2. The view should be something like:
<p>:Last post by: <%= post.user_id %> - You are: <%= current_user.id %></p>

@cdeyoung
Copy link

cdeyoung commented Sep 2, 2013

Thanks for your help, Chris. My _notify.html.erb was in the refetch directory. I updated my original post to show that.

@MhdSyrwan
Copy link
Contributor

@chrismccord Sorry for my late reply, I'll package that as a pull request soon :) .

@aaronjensen
Copy link
Contributor

FWIW @cdeyoung I'd recommend against refetch and just use client side to handle this. Have a custom Sync handler, include the user id in a data attribute on the view, and highlight it if the current user (also stored somewhere on the dom) is the same as the user in the user id. I think that this solution works quite well for pretty much everything that is individual user specific and, for me, does away with the need for refetch.

@cdeyoung
Copy link

cdeyoung commented Sep 3, 2013

@aaronjensen Thanks for the tip. I ended up doing this on the client side since it offered a lot more flexibility.

@ajb ajb closed this as completed Dec 22, 2014
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