ruby-hyperloop / hyper-mesh

The project has moved to Hyperstack!! - Synchronization of active record models across multiple clients using Pusher, ActionCable, or Polling
https://hyperstack.org/
MIT License
22 stars 12 forks source link

server_method macro looking for foo?= method #27

Open adamcreekroad opened 7 years ago

adamcreekroad commented 7 years ago

I defined a method on my model:

server_method :past_due? do
  ...
end

Nothing is broken on the client, but I get this message:

Warning: Deprecated feature used in Job. Server side method past_due?=(true) must be defined using the 'server_method' macro.

It seems it's trying to call past_due?= in addition to past_due?.

Here's where it's happening (in Opal)

if (method['$==']("id")) {
} else {
  target.$send("" + (method) + "=", value.$first())
}
catmando commented 7 years ago

I am thinking server_method is deprecated anyway in favor of using operations? I can't remember

sfcgeorge commented 6 years ago

I'm also seeing this and it's pretty jarring. The remote_method does seem to work, there's just that warning. If I make foo= a server method then it stops working. Likewise if I define an attr_writer. So I can't find any way to get rid of the warning.

There may be a good argument for removing model server methods to encourage smaller model files and custom logic to go elsewhere. Plus having to add Hyperloop specific code to models feels a bit wrong.

But for my use case I can't see a better way:

So advantage of server_method is it's automatically batched and done in the same request when loading a relation. As far as I can see there's no way to batch Operations other than making a parent operation that triages the data but that would be very complex and still a second request after the relation load.

janbiedermann commented 6 years ago

this apparently happens for any method that is not an attribute or somehow else defined. Client fetches by putting the method in the vector, lest's say the method is :past_due? When the data arrives, the value is assigned to the client RR "attribute/method/whatever", using '=', so you get: :past_due?=

so its maybe 2 bugs here.

  1. On reception of data client does not differentiate between attribute and server method properly
  2. method might be auto generated rails method, like :is_admin? for a boolean attribute :is_admin, value should then be applied to the attribute :is_admin using :is_admin= not :is_admin?=

I also find server_method very useful, its simple to use, little code to write, big value for development.

I will remove the deprecation message for lap12 and try to fix the behaviour, if nobody objects.

janbiedermann commented 6 years ago

if fixed the behaviour as follow: method_missing in hyper-mesh/lib/reactive_record/active_record/instance_methods.rb

the ! is cared for as it was.

1. Server Method

If the method is a server_method its name is left alone and values are set with name= and no warning is printed. example:

server_method :past_due?
stored name in server_methods and RR attributes: 'past_due?'
value set by RR with: past_due?=

2. Attribute

if the method is a attribute, its more AR like now, for booleans that is:

attribute: :is_admin
AR auto generated method: is_admin?
stored name in attributes and public_columns_hash: 'is_admin'
value fetched with method is_admin?: is_admin
value set by RR with: is_admin=

3. Not Attribute, Not Server method

deprecation_warning is printed to console method is called just with its name, whatever the result will be and

the method_missing got a little longer, but i removed the regexp so performance is still good enough, this method_missing is not hit so often.

https://github.com/ruby-hyperloop/hyper-mesh/blob/9b5dcf14a7eefc4fcd4921c3a5a95597d5ac0b8f/lib/reactive_record/active_record/instance_methods.rb#L88

will be done in lap12, ill close this

sfcgeorge commented 6 years ago

I'm using a server method like an attribute in order to do some additional processing on a field but still have Hyper Mesh automatically update the frontend. This is what I did to get the warning to go away:

# model
class Post < ApplicationRecord
  # attribute :text

  server_method(:text_data) { text }

  if RUBY_ENGINE != "opal"
    def text_data=(data)
      self.text = Base64.decode64(data)
    end
  end
end

# component
record = Post.first
record[:text_data] = "14d5a2ff9"
record.save.then do
  # do stuff
end

You may wonder why, I'm actually using this method to do Carrierwave file uploads. Yep.

catmando commented 6 years ago

@sfcgeorge so my understanding is that in the above all the server_method does is get the warning to go away in a very artificial manner. Is that correct?

if so it would seem that we just want to make server_method smart enough to understand what server_method("text_data=") { |data| self.text = Base64.decode64(data) } means.

sfcgeorge commented 6 years ago

@catmando Yeah it is just to get rid of the error. Without suppressing the warning a million lines of base64 image data get printed to the browser console and Chrome freezes.

What I'm doing seems a bit hacky, but at the same time quite elegant because you get all the automaticity of hyper-mesh; validation errors; state updating; etc, and it's pretty simple but works great.

Yes quite possibly, I tried server_method("text_data=") and other variations but that didn't work right.

catmando commented 6 years ago

@janbiedermann in your redo of the AR method_missing code, you automatically treat assignment to a server method as writing to an attribute. Any reason why?

My proposal is to allow server_methods to end in = and if they do then it will

  1. define the foo= method on the server per the supplied block
  2. write to a hidden attribute foo on the client, which will get sent to the server on save.

I'm not 100% happy with this, but...

janbiedermann commented 6 years ago

When i interpret my above comments correctly, to answer the why @catmando , then probably because it was like that and value set by RR with = hints that somewhere in RR code server_methods are treated as attributes and the value is assigned with = on the client. That it was like that and that it was the original intention of treating server_methods as attributes is also suggested by the opening post of this issue.