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 cross-scope/iframe support #153

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@studgeek
Copy link
Contributor

studgeek commented Sep 12, 2011

Allow observables to work across frames by replacing the function identiity comparisons in isWriteableObservable and isObservable with duck typing.
See https://groups.google.com/forum/#!starred/knockoutjs/DTIdBZeDA8U for background.
All tests (spec/qunit) pass.

@studgeek

This comment has been minimized.

Copy link
Contributor

studgeek commented Sep 13, 2011

Note I think this allows you to remove all of the ko_proto stuff, but I didn't want to delete it in case it served some other purpose I was missing.

studgeek added some commits Sep 12, 2011

Allow observables to work across frames by replacing the function ide…
…ntity comparisons in isWriteableObservable and isObservable with duck typing.

See https://groups.google.com/forum/#!starred/knockoutjs/DTIdBZeDA8U for background.
All tests (spec/qunit) pass.
Updated domNodeIsAttachedToDocument to be more efficient and also wor…
…k across document/iframe scopes. Added test for domNodeIsAttachedToDocument (passes before and after change).
@studgeek

This comment has been minimized.

Copy link
Contributor

studgeek commented Oct 3, 2011

I also just added a commit that changes domNodeIsAttachedToDocument to be more efficient and also work across scopes/iframes.

@hellige

This comment has been minimized.

Copy link

hellige commented Oct 3, 2011

I'm curious about the status of this pull request. I really want cross-frame support and would love to know whether it's likely in the near future. Any thoughts?

@hellige

This comment has been minimized.

Copy link

hellige commented Oct 4, 2011

Maybe I misunderstood how ambitious this patch was... I've just tried a simple example of an observable shared between the top window and an iframe, and the iframe view is still not updated based on changes to the shared observable. studgeek, do you feel like this patch should enable that behavior, or was I hoping for too much?

@hellige

This comment has been minimized.

Copy link

hellige commented Oct 4, 2011

Never mind, I did get it working. It's just that it remains necessary to be extremely careful about which frames include knockout.js and which context elements are used when applying bindings. It would be great if knockout could be made less finicky about this stuff in future releases, but for the time being this patch is an enormous improvement. I do hope you'll merge it for 1.3!

@studgeek

This comment has been minimized.

Copy link
Contributor

studgeek commented Oct 4, 2011

Yes its tricky, and there really is more to do. It probably needs some tests around iframes. Here are the problems I know about (at least off the top of my head), unfortunately I do not have solutions for all of them. For my descriptions I am using the following abbreviations for the chunks of behavior that can live in different frames:
KO binding - the graphical/DOM part of KO
KO observables - the ViewModel, observables part of KO
G - the graphics/DOM part of the user code
M - the model/data part of the user code

  1. isObservable uses a function identity test
    This means that KO observables created in M will not work in a separate G (because G doesn't know they are observables).
    My first commit fixes this. I also think its a bit faster/cleaner than the existing approach and would allow more cleanup based on it also.

  2. domNodeIsAttachedToDocument checks to see if the node is in the "current" document
    This means that if you have KO bindings (and their generated observables) defined in M and try to use it in a separate G's DOM then all observables will be automatically disposed. This happens because the M's KO binding's observables think their elements have been removed since those elements are not in M's document.
    My second commit fixes this. Instead of testing against "document" (which would be M's document in this case), the code goes up the tree from G's element to see if its contained by a document. I also think it will be a bit quicker then the existing top-down approach.

  3. observable discovery uses a _frames variable that is stored in iframe-specific scope
    This means that if you have a dependencyObservable in G that indirectly uses an observable in M, then M's observable it will not be added to the subscription list of G's dependencyObservable (because it is added to M's _frames variable which G can't see) and therefore it will not update on changes in the M observable.
    I haven't thought of a way to fix this other than some sort of setup step that sets where the _frames global is accessed.

  4. KO evaluates bindings in the iframe scope of the current call chain rather than the node's iframe scope
    This means if you define KO in M then use that instance to call applyBindings on elements in G, those bindings will be evaluated in the scope of M. So they will see M's jQuery rather than G's jQuery.
    I've looked at updating the evalInScope code to add G's node's window to the evaluation scopes, and this works. But I have not tried it out enough to submit it.

  5. "compile-and-go" issues if an observable is created from G in M and then G goes away
    This one is a bit obscure, but I thought I would list it for completeness. What is happening here is that you create observables in G then store them on M then G goes away (page change) you run into issues because you have functions in M that were created in an old scope (G). There is also a Firefox bug that is making this much worse because its pickier about what functions can exist after G goes away than it should be.

All of the above has me thinking the following for approaches:

  • KO, G and M all in the same scope - works, which isn't a surprise. One solution then is to copy the M to each scope, but then you have to find a to sync M between scopes.
  • KO binding & G in one iframe, KO observables and M in 2nd iframe - fails due to #3 (not sure how to fix)
  • G in one iframe, KO bindings and observables and M in 2nd iframe - fails due to #4 (but might be fixable and I think is the best bet for a single KO instance across multiple iframes)

My current approach is to have a an instance of KO each G iframe, then writing my own glue code based on my own M events (loosely based on Backbone) to connect each G to M. It requires less KO hacking, but more work on my side outside of KO. We will see how it goes...

@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Oct 11, 2011

Thanks for this. It looks like a pretty ambitious and far-reaching change, so I'll schedule it for possible inclusion in version 1.3.1. I may have to ask more questions and suggest changes at that point.

@hellige

This comment has been minimized.

Copy link

hellige commented Oct 30, 2011

I wanted to thank you both for the replies, especially studgeek for the very thoughtful and detailed comments. With these patches, I've been pretty happy with my arrangement, enough so that I've moved on to work on other things for the time being. But I wanted to mention my current approach, in case it's helpful for others...

As I said, I'm using the patches in this pull request. In my situation, I'm building what really amounts to a single cohesive app, using iframes mostly just to achieve modularity and allow some flexibility in composing different views. Therefore, I have a main "app" library of my own that I load in each frame, but actually expose only in the top window. My app wraps the top-window instance of ko and exposes the few ko functions that I need as functions on itself. This way I'm careful to only ever use the single top-window ko to create/bind observables, no matter which iframe I'm in.

Regarding your problem 4 above (G's jQuery vs. M's jQuery)... the only place I've had a problem with this is when using templates. I surmounted this problem with a horrible but easy hack... I have a convention for identifying template elements (easy enough) and on frame load, my "app" library locates all templates in the frame and moves them (or copies them, can't recall) into the head of the top window. Then the top jQuery is able to find them and all is well (although obviously I need to be careful that template IDs are globally unique). This sounds horrible, but it's really only three lines of code in one place.

I'm sure there are other problems, but as I said, this approach worked well enough that I haven't had to think about it any more. On the other hand, it's finicky enough and feels unsupported enough that when I revisit this code, I may well try to ditch the iframes entirely and move to dynamically loaded div content or something similar. I think it'll be somewhat less modular but may well be simpler and less error-prone in the long run.

Anyway, thanks again for the patches and for putting so much thought into this issue!

@studgeek

This comment has been minimized.

Copy link
Contributor

studgeek commented Oct 31, 2011

Hi hellige,

First of all, I tweaked my post so it was clearer what I mean by the KO, M, and G stuff. Hopefully that will make it easier for others to read.

What you are doing sounds like one good 4th approach. From what I can tell you are keeping KO, jQuery, and M together in a top frame. And you have multiple Gs in sub-frames. So it sounds like #2 and maybe #1 are all you need. You avoid most of the other problems by having everything together (including jQuery) in the top scope which is the parent document and also guaranteed to live longer than the sub-frame Gs.

Its worse for me because I am doing an extension, so my top frame is the browser which is XUL - and that has its own jQuery issues. Still, I know I am an edge case so your situation is probably the better target.

d

@Cybermotron

This comment has been minimized.

Copy link

Cybermotron commented Dec 27, 2011

I just wanted to say that I'm loving this. I've been using it in developing my latest app which requires 4 iframes all bound to the same viewmodel and its working a treat. I really hope this can be integrated back into the trunk soon so I can take advantage of all the latest developments over there.

Awesome work!

@jeremyhicks

This comment has been minimized.

Copy link

jeremyhicks commented Feb 16, 2012

Knockout noob here and I was struggling with getting knockout to apply to some bindings within an iframe while my model was in the containing page's js. I was ecstatic to get it working with just native knockout 2.0 code. I'm note sure of the complexity and changes that needed to happen for it to work but as a new guy coming into KO it bolstered my productivity while cleaning out large blocks of code related to logic / display triggered by events. Thanks!

@mbest mbest referenced this pull request Feb 29, 2012

Closed

2.1 release discussion #338

@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Mar 21, 2012

OK, sorry it's taken a long time to get to review this in detail. Thanks for the original code and discussion.

The trickiest thing about this is that KO needs a few bits of global state in order to keep track of what's going on (e.g., for dependency detection, for disposal, plus there are some caches). Should these be able to reach out, discover instances of KO in other frames, and find ways of cooperating and synchronising with them? Maybe that would be desirable if your goal is to make the page behave as a single document. But it certainly wouldn't be desirable if your goal is to use frames as a unit of isolation, and have completely separate worlds (maybe different KO versions) in each frame.

I'd say it isn't for KO to decide whether or not frames are isolated. It's up to the developer to choose that. Fortunately it's quite easy: by default, frames are isolated of course, but if you want and you have multiple frames on the same domain, you can simply pass the top-level ko object from one frame to another. Then it's just a single shared KO instance, and you don't have any issues with isObservable etc. I think this is a lot cleaner than trying to make distinct KO instances cooperate, because for one thing it guarantees you don't have a version mismatch, and it ensures all the internal state (caches, etc.) operates as designed and doesn't lose track of what's happening in a different frame.

Here is an example: http://jsfiddle.net/eZMTM/ - the master frame references KO, and passes a copy into the child frame. The child frame doesn't even reference KO, but uses the instance it receives from the master frame.

The only change needed to make this work is to the domNodeIsAttachedToDocument function, as studgeek discovered. I've put a 1-line implementation in the branch 153-cross-frame-disposal (commit a68262b).

What do you think? Is that sufficient to give enough control over the isolation/sharing?

@mbest

This comment has been minimized.

Copy link
Member

mbest commented Mar 22, 2012

Steve, I like it.

I thought of another way to initialize the iframe's bindings, using a binding (included below). Here's your example updated to use this binding: http://jsfiddle.net/mbest/GYRUX/ The binding will automatically re-bind if the inner frame is reloaded or navigated.

ko.bindingHandlers.bindIframe = {
    init: function(element, valueAccessor) {
        function bindIframe() {
            try {
                var iframeInit = element.contentWindow.initChildFrame,
                    iframedoc = element.contentDocument.body;
            } catch(e) {
                // ignored
            }
            if (iframeInit)
                iframeInit(ko, valueAccessor());
            else if (iframedoc)
                ko.applyBindings(valueAccessor(), iframedoc);
        };
        bindIframe();
        ko.utils.registerEventHandler(element, 'load', bindIframe);
    }
};
@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Mar 22, 2012

OK great, here's the simplest pull request ready to be merged in then: #404

I thought of another way to initialize the iframe's bindings, using a binding (included below)

That's cool. It would be a good addition to the "recipes" page (https://github.com/SteveSanderson/knockout/wiki/Recipes).

@rniemeyer

This comment has been minimized.

Copy link
Member

rniemeyer commented Mar 22, 2012

Good stuff. I can see some interesting possibilities for it.

@SteveSanderson

This comment has been minimized.

Copy link
Contributor

SteveSanderson commented Mar 23, 2012

Closing this pull request in favour of 153-cross-frame-disposal-with-named-templates

@hellige

This comment has been minimized.

Copy link

hellige commented Apr 13, 2012

I just wanted to say that I tested the changes in 153 and 154 and, at least for our use case, they do not make cross-frame support work. So something from this patch has definitely been lost. Unfortunately, it was the last straw for us, so I've rearranged our app not to require iframes at all. So I'm happy at this point, but I did want to state for the record that I'm not sure that the new changes actually address this issue.

@mbest

This comment has been minimized.

Copy link
Member

mbest commented Apr 13, 2012

We did not try to support all possible cross-frame scenarios. Specifically we did not try to support two instances of Knockout co-mingling. In order to work properly with Knockout, both frame have to share a single instance. See the example from Steve above for how to do that.

@hellige

This comment has been minimized.

Copy link

hellige commented Apr 13, 2012

But that is exactly what our app did. We shared a single instance of knockout in the top window, see my last comment above for details.

I couldn't figure out exactly where the new version was going wrong, but I can say that it definitely did not work. If it works for the scenarios other people care about, that's what matters, but I figured it was worth noting that it does not fully replace the functionality in this pull request, since others may have the same problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment