Fix problems with skipped _notifyPath forwarding #60

warpech · 2017-10-18T15:06:07Z

@atifwaqar reported to me on Slack some problems with partial view replacement. This PR fixes the problem and adds tests for the things that were fixed.

It is explained in more detail in the commit messages.

An optimization in _notifyPath causes notifications not to be forwarded to imported-template if the value passed to _notifyPath is an object that is the same as the existing model. This is a legitimate case, in which dom-bind "resets" the object in order to inform that one of the properties has changed Solution: always forward the notifications if the model is the same instance. Also fixed a side problem that appeared while fixing this. Starcounter-include forwarded notifications to imported-template before the template was stamped

…s resets shadowRoot in partialChanged, even if not needed Solution: if the instance of defaultComposition is the same as previously used, skip stamping the composition

alshakero · 2017-10-18T21:50:15Z

starcounter-include.html

@@ -246,6 +247,7 @@
         * @param  {Mixed} value New value
         */
        StarcounterIncludePrototype._notifyPath = function(path, value) {
+            var modelAlreadyLoaded = this.template.model == this.partial;


EDIT: you can ignore this comment. After a second thought, both points aren't valid.

I think there is a chance when _notifyPath is called before stamping (this.template assignment). Maybe this.template && this.template.model == this.partial would be more tolerant.

What about null == null? Maybe even

this.template && this.template.model && this.template.model == this.partial

I'd rename it to sameModelAlreadyLoaded as current name gave me lots of confusion in reasoning mentioned below.

That's a fair point.

alshakero · 2017-10-19T08:21:37Z

test/partialAttribute/nested-html/stamped_from_polymer_template.html

+                    importedTemplate.addEventListener('stamping', function onceStamped(){
+                        importedTemplate.removeEventListener('stamping', onceStamped);
+                        scope = domBind.get('model.Page.scope1');
+                        scope.doesItWork = newVal;


Is this line 197 needed?

You are right, this line doesn't add anything meaningful to the test. I will remove it.

alshakero

Look great. I can take care of it from here if you want.

Spotted by Omar in #60 (review) Solution: remove that line.

warpech · 2017-10-19T09:07:07Z

I would be grateful if you could release this as a next version of starcounter-include and put it to Starcounter v2.3.

@Mihaiii says on slack that this PR also fixes his problem:

the search problem of i.s.c is actually solved with this PR: #60 , not what I thought initially.

alshakero · 2017-10-19T09:20:32Z

Sure!

tomalec

@alshakero @warpech
Have we changed the design decisions from #34 (comment)?

I thought we agreed to:

In case of the change of one sibling, we reload the HTML (partialChanged) and DO NOT notify the existing view with new model, until it's loaded (NO sequential _notifyPath

starcounter-include/starcounter-include.html

Lines 283 to 288 in c4ce900

    
           if (modelAlreadyLoaded && this.template._notifyPath) { 
        
               this.template._notifyPath( 
        
                   path.replace("partial.", "model.") 
        
                       .replace("viewModel.", "model."), 
        
                    value); 
        
           }

- your modelAlreadyLoaded check will pass it ),

In case of just nested property change model.scope1.some.prop, we DO NOT touch either URL (_htmlChanged) nor composition (_compositionChanged).
Means no partialChanged in

starcounter-include/starcounter-include.html

Lines 283 to 288 in 8725dff

    
           } else if (this.template._notifyPath) { 
        
               this.template._notifyPath( 
        
                   path.replace("partial.", "model.") 
        
                       .replace("viewModel.", "model."), 
        
                    value); 
        
           }

Maybe checks in individual _htmlChanged, _compositionChanged made tests passing, but running them, in my opinion, is unnecessarily complicating the flow and performance.

tomalec · 2017-10-21T08:36:37Z

starcounter-include.html

+            } else {
+                // make clone to avoid direct binding
+                composition = this.stringToDocumentFragment(compositionString);
+            }


Why was it moved here? Was it causing any problems? Now it's calling composition = this.stringToDocumentFragment(compositionString); even though there may be no need for.

You are right about this. Actually, I have created a regression that was not caught by tests. I will make another PR.

tomalec · 2017-10-21T08:45:24Z

starcounter-include.html

+                                path.replace("partial.", "model.")
+                                    .replace("viewModel.", "model."),
+                                 value);
+                        }


Isn't it the regression of #37 (ignoring Palindrom meta-data)?

At least performance-wise even thought no attrs for i-t are being changed we may still ping _notifyPath and partialChanged after each version bump.

Isn't it the regression of #37 (ignoring Palindrom meta-data)?

There is a test for that (see the PR https://github.com/Starcounter/starcounter-include/pull/44/files), so I don't think so?

At least performance-wise even thought no attrs for i-t are being changed we may still ping _notifyPath and partialChanged after each version bump.

If no attrs for i-t are being changed than why was s-i._notifyPath was called in first place? If some performance optimisation should be applied, I believe it should be done outside of s-i, not inside of it.

There is a test for that

The test you mention, tests building URL, not propagating changes to nested templates.
So there is unfortunatelly no test for that.

If no attrs for i-t are being changed than why was s-i._notifyPath was called in first place?

Consider the case:

viewModel = { _ver#s: 1, _ver#c$: 1, App: {} }

<dom-bind><template> <starcounter-include view-model="{{viewModel}}"/> </template></dom-bind>

// palindrom-client translates `{op: 'replace', path: '/_ver#s', value: 2}` to polymer notification domBind.set('viewModel._ver#s', 2); // palindrom-client translates `{op: 'replace', path: '/App', value: {}}` to polymer notification domBind.set('viewModel.App', {});

In both cases no attributes for i-t are being changed. But starcounter-include is called for both.

I believe it should be done outside of s-i, not inside of it.

Like, where? s-i is supposed to do all Starcounter specific magic to untangle merged view-models. I consider Palindrom metadata part of it.

I am not sure if you mean that it is wrong behavior or performance problem?

The test that I mentioned was checking for wrong behavior. What you suggest seems to be a performance improvement and I agree with it, though I am not sure about the significance of it.

It is not just performance improvement.

The case I described is the correct behavior, that is not tested and might be broken by this change.

Why do you think https://github.com/Starcounter/starcounter-include/pull/44/files tests wrong behavior?

Why do you think /pull/44/files tests wrong behavior?

Sorry for choosing wrong words.

The test in https://github.com/Starcounter/starcounter-include/pull/44/files is correct. It tests in the metadata is ignored by the URL builder (which is desired). It does not check if notifications about the metadata are forwarded or skipped (which doesn't matter except for performance).

Why do you think that it matters if we skip forwarding the notifications about the metadata?

You are right. Currently it's only performance, and it's not notified any further due to functionality we lost while upgrading to Polymer 2.0 Juicy/imported-template#43.

tomalec · 2017-10-21T08:46:53Z

test/composition/declarative-shadow-dom.html

+                scInclude.partial = scInclude.partial;
+                setTimeout(function() {
+                    expect(old).to.be.not.null;
+                    expect(old).to.be.equal(scInclude.shadowRoot.querySelector("h2"));


I'd rather compare entire DocumentFragment to make sure no changes were made to .shadowRoot

Do you mean compare old scInclude.shadowRoot to the current scInclude.shadowRoot? Wouldn't this test a different thing? In this test I am testing exactly what is needed to make https://github.com/Starcounter/Website/pull/211 work, comparing the root objects would not test that.

But that way, you are not testing "keep", you are testing "is again/still the markup". I think we should avoid flashing and in case there is a custom element that animates something, for example, we shouldn't break it.

No, line 89 checks the equality of the DOM node, not just the equality of the node's name.

Perhaps we could check for flashing caused by detaching and attaching the same node, is that what you mean? I guess a way to do that would be to use mutation observer.

The fact that one particular node is back there again does not mean it didn't flashed due to:

detaching and attaching the same node,

attaching some other nodes,

removing some other nodes,

removing entire composition, putting another one, then restoring this back.

Shadow DOM (composition) is a DocumentFragment and that's why I believe we should treat it as a whole, not just individual elements.

I guess a way to do that would be to use mutation observer.

Great idea. :)

But would comparing DocumentFragment would test against flashing? I think we can only test flashing with mutation observer, no?

Anyway, feel free to improve the test. My goal was to add a test for something that failed before. I didn't try to solve anything more than that.

tomalec · 2017-10-21T08:49:16Z

test/partialAttribute/nested-html/stamped_from_polymer_template.html

@@ -92,11 +92,12 @@
                    done();
                });
            });
-            it('should forward Polymer notification to `imported-template`', function () {
+
+            it('should NOT forward Polymer notification to `imported-template` before it was stamped', function () {


Aren't we taking responsibility of imported-template here? When I wrote it for the first time, my idea was that sc-include forwards all data and notifications to imported-template as soon as it gets it. Not, to block imported-template from consuming that in the way it needs.

I found this inconsistent:

the test above (line 96) checks that Polymer notification is forwarded before the template is stamped

the tests like

starcounter-include/test/partialAttribute/nested-html/given_in_HTML.html

Line 76 in c4ce900

describe('should attach to `<imported-template>` concatenated `Html`s as href attribute and property (`/sc/htmlmerger?`+`scope=scope.HTML` for each scope that contains it) and attach partial\'s model only once content is stamped', function () {

check that model is not assigned before the template is stamped

For me this is contradicting. The fix that I made 1acb2e9 (with a new test added) caused the test at line 96 to fail, and my conclusion was that the test at line 96 was wrong and should be inverted.

tomalec · 2017-10-21T08:51:58Z

test/partialAttribute/nested-html/stamped_from_polymer_template.html

@@ -183,6 +184,26 @@
                    });
                });
            });
+
+            describe('after `dom-bind` re-set the sub-partial property (just one scope) to the same instance but potentially with new values', function () {


After you removed scope.doesItWork = newVal; it's no longer "potentially with new values".

This test only checks if the notification is forwarded, without really testing any values in it. Perhaps the description should be changed.

Well.. it was also involving testing integration with Polymer's dirty-checking. But Polymer 2.0 does not forward it anyways, so we already change this test to check that it's NOT forwarded

tomalec · 2017-10-21T09:04:36Z

starcounter-include.html

@@ -246,6 +247,7 @@
         * @param  {Mixed} value New value
         */
        StarcounterIncludePrototype._notifyPath = function(path, value) {
+            var modelAlreadyLoaded = this.template.model == this.partial;


I'd rename it to sameModelAlreadyLoaded as current name gave me lots of confusion in reasoning mentioned below.

…yPath resets an unsaved explicit composition created using CompositionEditor Solution: change the _compositionChanged method to more clearly implement the complex process of selecting which composition to apply. Fix explicit composition caching so it does not get overwritten by fallback composition. Add a test

warpech added 2 commits October 18, 2017 15:30

Problem:when default composition is in use, starcounter-include alway…

1acb2e9

…s resets shadowRoot in partialChanged, even if not needed Solution: if the instance of defaultComposition is the same as previously used, skip stamping the composition

warpech requested a review from alshakero October 18, 2017 15:06

alshakero reviewed Oct 18, 2017

View reviewed changes

alshakero reviewed Oct 19, 2017

View reviewed changes

Problem: The test contained a line that does not change anything

41ba47c

Spotted by Omar in #60 (review) Solution: remove that line.

alshakero merged commit a4ea7d9 into master Oct 19, 2017

tomalec reviewed Oct 21, 2017

View reviewed changes

warpech mentioned this pull request Oct 24, 2017

Improvements after PR #60 #62

Merged

alshakero added a commit that referenced this pull request Oct 24, 2017

Merge #62: Improvements after PR #60

9af1b5f

tomalec mentioned this pull request Feb 22, 2018

Add tests for FOUC #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix problems with skipped _notifyPath forwarding #60

Fix problems with skipped _notifyPath forwarding #60

warpech commented Oct 18, 2017

alshakero Oct 18, 2017 •

edited

Loading

tomalec Oct 21, 2017

warpech Oct 22, 2017

alshakero Oct 19, 2017

warpech Oct 19, 2017

alshakero left a comment

warpech commented Oct 19, 2017 •

edited

Loading

alshakero commented Oct 19, 2017

tomalec left a comment •

edited

Loading

tomalec Oct 21, 2017

warpech Oct 23, 2017

tomalec Oct 21, 2017

warpech Oct 22, 2017

tomalec Dec 30, 2017 •

edited

Loading

warpech Dec 30, 2017 •

edited

Loading

tomalec Dec 30, 2017 •

edited

Loading

warpech Jan 2, 2018 •

edited

Loading

tomalec Jan 2, 2018

tomalec Oct 21, 2017

warpech Oct 22, 2017

tomalec Dec 30, 2017

warpech Dec 30, 2017

tomalec Dec 30, 2017

warpech Jan 2, 2018 •

edited

Loading

tomalec Oct 21, 2017

warpech Oct 22, 2017

tomalec Oct 21, 2017

warpech Oct 22, 2017

tomalec Dec 30, 2017 •

edited

Loading

tomalec Oct 21, 2017

	if (modelAlreadyLoaded && this.template._notifyPath) {
	this.template._notifyPath(
	path.replace("partial.", "model.")
	.replace("viewModel.", "model."),
	value);
	}

	} else if (this.template._notifyPath) {
	this.template._notifyPath(
	path.replace("partial.", "model.")
	.replace("viewModel.", "model."),
	value);
	}

Fix problems with skipped _notifyPath forwarding #60

Fix problems with skipped _notifyPath forwarding #60

Conversation

warpech commented Oct 18, 2017

alshakero Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero left a comment

Choose a reason for hiding this comment

warpech commented Oct 19, 2017 • edited Loading

alshakero commented Oct 19, 2017

tomalec left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

warpech Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

tomalec Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

warpech Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warpech Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero Oct 18, 2017 •

edited

Loading

warpech commented Oct 19, 2017 •

edited

Loading

tomalec left a comment •

edited

Loading

tomalec Dec 30, 2017 •

edited

Loading

warpech Dec 30, 2017 •

edited

Loading

tomalec Dec 30, 2017 •

edited

Loading

warpech Jan 2, 2018 •

edited

Loading

warpech Jan 2, 2018 •

edited

Loading

tomalec Dec 30, 2017 •

edited

Loading