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.

Sign up for GitHub

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

Jump to bottom

Computed observables with independent top-linked 2-way data-binding overwrite each other when using same template/tagExpr #287

Closed
Paul-Martin opened this issue Nov 1, 2014 · 10 comments
Closed

Computed observables with independent top-linked 2-way data-binding overwrite each other when using same template/tagExpr #287

Paul-Martin opened this issue Nov 1, 2014 · 10 comments

Comments

@Paul-Martin
Copy link

Hello Boris,

Sorry for the confusing title, but I can't seem to succinctly describe the issue.

It seems that when two independently linked views, using two different instances of the same class of computed observables, with an observable path >= 2 are used on the same page, the view which is linked second will overwrite the data of the first view. For example, using this template:

<script id='field' type="text/x-jsrender">
   <input data-link="{:a().b() onerror='' trigger=true :}"> <span data-link="{:a().b()}"</span>
</script>

with two independent divs like this:

<div id="one"></div>
<div id="two"></div>

and two different instances of the same objects like this:

var o1 = new Foo(), o2 = new Foo()

and then linking each instance to each div:

$.templates.field.link('#one', o1);
$.templates.field.link('#two', o2);

will result in the input field in div#one being linked to o2.

You can see a working example at this fiddle:
http://jsfiddle.net/PaulMartin/qn4mucxk/

I've spent quite a bit of time trying to understand what is happening and it appears to me that when o2 is linked to div#two, a lookup on view.tmpl.links[tagExpr] is performed and .deps is updated with the o2 values since both have the same tagExpr. Unfortunately I haven't been able to isolate exactly where and when this happens. I'll continue to look at it and update this issue if I find more information.
@Paul-Martin
Copy link
Author

Hi Boris,

I was able to work on this more today and I believe I have a fix for this issue. Essentially I've modified the View constructor to make a shallow copy ($extend({}, template)) of the template, and then check for and replace the links hash if it exists.

        self.tmpl = template && $extend({}, template);
        if (template && template.links)
            self.tmpl.links = {};

I also updated all callers of the View constructor to replace their working copy of tmpl immediately after the call to new View.

// .... newView = ... new View(....)
tmpl = newView.tmpl;

All the test cases in the UNIT-TESTS-ALL-JSVIEWS.html file pass after these changes and this issue is resolved with them in place.

I'm working on adding some test cases to cover this scenario and then I'll submit a pull request to you for your review.

I also noticed a property called 'bnds' that is part of the template. I'm not clear on it's use, but it looked like it might be a property similar to 'links' that may also need to be view specific when used with computed observables. I'd appreciate any thoughts you might have on this property and I could make similar changes for that if you think that is necessary.

if you have any other thoughts on what I've done, please let me know.

Thanks!

@BorisMoore
Copy link
Owner

That's good to hear, Paul. Thanks. I was planning to start investigating this issue soon. Thanks very much for reporting it. It is great that you are using these computed scenarios extensively and giving such valuable feedback.

Yes, by all means send me a pull request for review - or if you prefer, email me a copy of your jsviews.js and I can do a diff and take a look...

If you choose a pull request, would that be on the jsviews.com project? For me that is the primary project where I work, then I copy over the relevant files to the jsviews and jsrender projects when I am ready to commit...

The bnds property may also have a similar issue of being 'erroneously shared' across instances. It is essentially the set of tag bindings within the template. I need to check on whether there is instance-specific state included there...

Paul-Martin pushed a commit to Paul-Martin/jsviews that referenced this issue Nov 2, 2014
Fix issue BorisMoore#287
b726903 
Update such that each instantiation (view)  of a template has it's
own tmpl.links object so that computed observables stored in the
tmpl.links.dep array are specific to that particular view and
not shared across views.

Added test cases to cover this scenario.
@Paul-Martin
Copy link
Author

Hi Boris,

I've just submitted a pull request with the changes and test cases. Initially I submitted to jsviews project, but have since resubmitted to the jsviews.com project. Upon further review the View class is actually part of jsrender, so I made the changes to both jsview.js and jsrender.js. I also included updated test cases. I did not attempt to regenerate any of the minimized files as I'm not certain what tool chain you are using. I attempted to follow your qunit style closely, though I did introduce my own test data. If it would be more convenient to receive the raw files via email, I'm happy to forward them that way as well.

@BorisMoore
Copy link
Owner

Excellent, thank you Paul. I hope to be able to investigate the issue, and study your fix, soon. It might be a few days though, for me to follow through, given some other commitments I have... I'll keep you posted.

@Paul-Martin
Copy link
Author

Hi Boris,

I was just rereading and I realized that I wasn't clear on what I fixed. I fixed the issue related to .deps, but I did not fix the issue related to .bnds as I don't have a valid test case for that scenario. Just wanted to clarify. I suspect the exact same fix will work, but I didn't want to make changes without being able to validate it.

No hurry on my part. My fix is working fine for me right now.

@BorisMoore
Copy link
Owner

Hi Paul,

Sure, makes sense.

I am looking at a fix that is different from yours, since your approach in effect, for many scenarios at least, removed the intended perf optimization of using the cached template in tmpl.links - leading to re-compilation. I would propose either to simplify code by removing that whole attempt at caching, or get it to work correctly as an optimization in the primary scenarios at least. The approach I am exploring tries to do the latter, but I need to find time to dig in a bit deeper in some related code paths before committing it. I'll keep you posted when I get there :).

@BorisMoore
Copy link
Owner

@Paul-Martin: Paul, just to let you know I have a fix for this ready, but I am also working on a possible fix for #285 - and hitting a few somewhat obscure bugs (for uncommon scenarios) around chained computed paths which I am also trying to address. So it may take a little while longer to complete the update with both fixes, but it is on its way...

@Paul-Martin
Copy link
Author

Thanks for the update Boris. Let me know if you'd like me to test anything pre-release.

@BorisMoore
Copy link
Owner

Yes, planning to do that. Still not got the final version though... Thanks Paul...

BorisMoore added a commit to BorisMoore/jsviews.com that referenced this issue Dec 25, 2014
@BorisMoore
Commit 61 (Beta Candidate)
640e42e 
- Improved support for computed values in paths: support
  and bug fixes for templates (and data-linking) which
  include complex expressions with computed properties
  (with or without parameters), including computed values
  which return objects, and using paths which chain
  computed values - for example:
  "a.b().c.d.e(p, q).f"

- Full support for deep paths with computed values, with
  live data binding to any chosen depth (see Breaking
  Change below), and optionally involving complex
  expressions - for example:
  "a.b()^c.d.e(p + (r - 1), q + 't').f + 'x' + g"

BREAKING CHANGE
(see BorisMoore/jsviews#285):
- Depth of binding of paths is now controlled by the '^'
  character, whether or not the path includes computed
  values.
  For example:
  "a.b().c"
  will do leaf binding only, so will 'observe' (listen to)
  changes in c, but NOT changes in b(). (This is breaking,
  since previously this path DID observe b()...)
  To observe both c and b(), use the path:
  "a.b()^c"
  To observe ALL changes in the path - a, b() and c(),
  use:
  "a^b().c"

- Improved support for custom tag inheritance from
  base tag - e.g.: {baseTag: "for", ...}

Bug Fixes
BorisMoore/jsviews#285
BorisMoore/jsviews#287
BorisMoore/jsviews#289

- Several small additional bug fixes.

- Many new unit tests added.
BorisMoore added a commit to BorisMoore/jsrender that referenced this issue Dec 25, 2014
@BorisMoore
Commit 61 (Post Beta)
19b74f4 
- Improvements and bug fixes for complex templates using
  computed properties, with or without parameters,
  including computed values which return objects, and
  using paths which chain computed values - for example:
  {{:a.b().c.d.e(p, q).f}}

- Full support for deep paths with computed values and
  optionally involving complex expressions - for example:
  {{:a.b().c.d.e(p + (r - 1), q + 't').f + 'x' + g}}

- Improved support for custom tag inheritance from
  base tag - e.g.: {baseTag: "for", ...}

Bug Fixes
BorisMoore/jsviews#285
BorisMoore/jsviews#287
BorisMoore/jsviews#289

- Several small additional bug fixes.
BorisMoore added a commit that referenced this issue Dec 25, 2014
@BorisMoore
Commit 61 (Beta Candidate)
3c83204 
- Improved support for computed values in paths: support
  and bug fixes for templates (and data-linking) which
  include complex expressions with computed properties
  (with or without parameters), including computed values
  which return objects, and using paths which chain
  computed values - for example:
  "a.b().c.d.e(p, q).f"

- Full support for deep paths with computed values, with
  live data binding to any chosen depth (see Breaking
  Change below), and optionally involving complex
  expressions - for example:
  "a.b()^c.d.e(p + (r - 1), q + 't').f + 'x' + g"

BREAKING CHANGE
(see #285):
- Depth of binding of paths is now controlled by the '^'
  character, whether or not the path includes computed
  values.
  For example:
  "a.b().c"
  will do leaf binding only, so will 'observe' (listen to)
  changes in c, but NOT changes in b(). (This is breaking,
  since previously this path DID observe b()...)
  To observe both c and b(), use the path:
  "a.b()^c"
  To observe ALL changes in the path - a, b() and c(),
  use:
  "a^b().c"

- Improved support for custom tag inheritance from
  base tag - e.g.: {baseTag: "for", ...}

Bug Fixes
#285
#287
#289

- Several small additional bug fixes.

- Many new unit tests added.
@BorisMoore
Copy link
Owner

This has been fixed in commit 61. Thanks Paul, for you help on calling this out, and working on getting a fix, or testing candidate fixes.

@BorisMoore BorisMoore mentioned this issue Dec 15, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@BorisMoore @Paul-Martin