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

Shouldn't Element be a Native? #24

Closed
elia opened this issue Aug 8, 2013 · 16 comments
Closed

Shouldn't Element be a Native? #24

elia opened this issue Aug 8, 2013 · 16 comments

Comments

@elia
Copy link
Member

elia commented Aug 8, 2013

with opal pointing to master any call to jquery plugins must be exposed explicitly (or wrapped)

cc @fkchang

@adambeynon
Copy link
Contributor

Yes, it should. But there are problems we need to address first. The main one is the type of object to return from native calls (via method_missing). For example, Element#find would be partially broken. Native#method_missing would see the result as a native object, but it would get wrapped as a Native, not an Element.

Sure, we could just use a plain wrapper for Element, but we are also adding custom methods. Which means we either need to have some clever way to detect what class a native object should we wrapped as, or, we have to manually do lots of coercion.

We could introduce properties to natives to say what classes they could become. e.g., we could add something like this to $.fn:

$.fn.toOpal = function() {
  return Opal.Element.$new(this);
};

And Native, when encountering a native object, could simply check if the object can coerce itself into the opal world. If not, just return a simple Native instance.

Just a thought.

@adambeynon
Copy link
Contributor

As an example:

class Native
  def method_missing(*)
    %x{
      // all logic here

      if (typeof (result.toOpal) === "function") {
        return result.toOpal();
      }
      return #{ Native(`result`) };
    }
  end
end

@adambeynon
Copy link
Contributor

As @meh suggests, perhaps Element#method_missing is a better place for this. It will let us have a lot more control over what happens.

@svoboda-jan
Copy link
Contributor

I tried patching Element to add back method_missing, but it is currently broken causing Uncaught TypeError: Object [object Object] has no method '$test_method'.

I relied a lot on this feature for jquery plugins.

@meh
Copy link
Member

meh commented Aug 13, 2013

Can you show me the code for the method_missing?

@svoboda-jan
Copy link
Contributor

I tried patching Element with the removed code ( 5e14389#L1L92 ):

class Element
  # Missing methods are assumed to be jquery plugins. These are called by
  # the given symbol name.
  def method_missing(symbol, *args, &block)
    %x{
      if (#{self}[#{symbol}]) {
        return #{self}[#{symbol}].apply(#{self}, args);
      }
    }

    super
  end
end

But even the following does not get called:

class Element
  def method_missing(symbol, *args, &block)
    p symbol
  end
end

Patching normal classes with method_missing works fine. Using opal v0.4.4.

@meh
Copy link
Member

meh commented Aug 13, 2013

So you're saying method_missing doesn't get called when doing Element.find('bleh').stuff?

@adambeynon
Copy link
Contributor

Element is currently bridged to jquery instances, so it can't support method missing as we don't control it's prototype. We need to turn it into a wrapper class (Native) to get real method missing working.

@svoboda-jan
Copy link
Contributor

Yes, and not even with Element.new.test_method.

@meh
Copy link
Member

meh commented Aug 13, 2013

@adambeynon oh, makes sense.

@svoboda-jan
Copy link
Contributor

@adambeynon Ok, thanks for the explanation. Would be nice to have Element#method_missing.

@adambeynon
Copy link
Contributor

As it turns out, trying to make Element act like a native is very tricky, and lots of awkward corner cases keep popping up. #on, #off, #find are particular pain points. For a short term solution, perhaps we should improve Element#expose which could manually add methods for these plugins you were using.

Element.expose :modal

Which would add an instance method for calling $().modal(). Its not as good as a method_missing solution, but Element needs a big rethink on how to handle wrapping jquery objects.

@svoboda-jan
Copy link
Contributor

For most of my current use cases the Element#expose is enough as it is now, so I can wait until progress is made on method_missing.

@adambeynon
Copy link
Contributor

As an update, I have (roughly) added native support to Element as per: 03dbe5d.

This commit basically means that instances of Element wrap an internal jquery object.

@adambeynon
Copy link
Contributor

So, turns out wrapping jQuery wasn't that great. Most methods needed to keep unwrapping/wrapping instances as before we just made the most out of toll-free bridging. For now, Element#expose is the way to go.

@elia
Copy link
Member Author

elia commented Oct 31, 2013

👍

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

4 participants