Third batch of commits for a review #79
Conversation
This may not be an obvious improvement and it certainly looks destructive. I checked the whole library and tried to test it where possible. It seems like it works well and doesn't break any assumptions when applied carefully. When implementing Iframe#content_window I had a realization that every next call to this function created a new Ruby object. I wondered if that's the best idea. I think it's also how jQuery encapsulates things. But we are not jQuery. I noticed that DOM::Node includes a #== override. That's nice, but Window is not a subclass of DOM::Node. There should have been some better way than adding a #== method in Native::Wrapper. I got inspired by the way we do .new dispatching in DOM::Node. Why not return there an existing object if it was already created. This way our Ruby DOM should kind of reflect the native DOM better. I thought about storing a map of references in some global variable, but I realized it would cause issues with a GC. Storing a reference to a Ruby object inside a JS object seems like a good compromise. The name Singleton usually means a class with a single instance. That may not be the best name here, but I couldn't find a better one. This change should offer some memory improvements under certain circumstances and under certain it won't.
This reverts commit 4f5d772. This commit isn't needed anymore since Native::Wrapper::Singleton.
opal/browser/utils.rb
Outdated
| # doesn't mind arbitrary properties. | ||
| # | ||
| # The rule of thumb is: if it does overload `#initialize`'s signature | ||
| # and not `.new`'s - it won't work. Use Native::Wrapper in this case. |
There was a problem hiding this comment.
It all makes sense, but it needs a couple of adjustments.
- It's better to stay within the
Browsernamespace and have a local, specific wrapper (that also helps in not polluting the constants space of every class that already includesNative::Wrapper) - As of the name I think something like
Browser::CachedNativeWrapper, but possibly something even better (please look for variations), as you saidSingletonis generally interpreted with another meaning, although it makes sense once you reason about it. - I would use something like
$$opal_browser_nativeor$$nativeinstead of__opal_singleton__because that wouldn't show up in instance variables, and it's the common practice in opal.
There was a problem hiding this comment.
I think this module isn't necessarily opal-browser only and maybe someday it would be wise to move it to some more general repo, like opal. After all, it's not tied to DOM, but for now it's most needed by DOM. I want to keep its name as similar to Native::Wrapper as possible (because some classes include one, others include the other), so maybe Native::CachedWrapper and $$opal_native_cached? If staying inside the namespace is more important, then Browser::NativeCachedWrapper will be the choice I will take for this pull request.
There was a problem hiding this comment.
By the way, the module has another ClassMethods module inside. I think it's the Ruby convention to do extends this way, but it will pollute the namespace anyway.
There was a problem hiding this comment.
So, the thing is, this looks like something that should end up in opal's stdlib. If by any chance the final version has a different api that opal-browser's version it will give us some trouble in reconciling the two, that's why I'd like to keep it in a namespace. The name you proposed and the desire to keep the names similar are 👍 so, what about Browser::Native::CachedWrapper?
WRT ClassMethods I'm not sure where it's coming from, but the stdlib module seems clean: https://github.com/opal/opal/blob/3eae98d70045b3daa56ca6656e3331c92ed18e57/stdlib/native.rb#L189-L212
There was a problem hiding this comment.
Good idea about the naming, I'm preparing the patch right now.
I override new which is a class method, so I need to extend it but simultaneously I want to include Native::Wrapper. The established way to do is to create a submodule and extend the class with this submodule inside self.included. Now I see that the submodule gets added as a constant as well. I moved it a level up and renamed it to CachedWrapperClassMethods. In a moment I will produce the commit.
There was a problem hiding this comment.
Oh wait, that's not really a good idea. This caused a lot of tests to fail with
undefined method `convert' for Browser::Native
And a few related errors like Browser::Native::Array etc. I will use the Browser::NativeCachedWrapper name.
This commit basically refactors the DOM::Element::Scroll module. This module was broken before, because there were multiple conflicting definitions of a #to function. I retained the classic one adding some syntactic sugar like $window.scroll.to :bottom. The other one got renamed to #into_view. My refactor integrated this code with Window::Scroll and removed this module afterwards. Inside this commit there are also minor caching changes for Window. After having implemented all that I noticed, that actually there are two DOM interfaces for Element.scroll* and Window.scroll* with a differing browser support. The thing we do is extracting a root Element from a Window so we don't need to use the Window interface at all (and it should be supported well cross-browser - and it's actually what we did partially with the Window::Scroll implementation before this commit).
As per @elia's suggestion, because, in his own words: "So, the thing is, this looks like something that should end up in opal's stdlib. If by any chance the final version has a different api that opal-browser's version it will give us some trouble in reconciling the two, that's why I'd like to keep it in a namespace" This commit also isolates ClassMethods to be located a level higher as Browser::NativeCachedWrapperClassMethods so as not to pollute the namespace of the including modules. This commit also renames the JS property name to "$$opal_native_cached" as per Opal current guidelines.
|
Just dropping by to say I'm happy someone is working on opal-browser 🐼 |
It's one of the most enjoyable projects I worked on :) |
No description provided.