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

Fix problems with skipped _notifyPath forwarding #60

Merged
merged 3 commits into from
Oct 19, 2017
Merged

Fix problems with skipped _notifyPath forwarding #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d4ea7e
Problem: Skipped notifications to imported-template
warpech Oct 18, 2017
1acb2e9
Problem:when default composition is in use, starcounter-include alway…
warpech Oct 18, 2017
41ba47c
Problem: The test contained a line that does not change anything
warpech Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions starcounter-include.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,14 @@
}
}
// do not change if there is one, it's same as before, and we are not forced
if (!compositionString) {
composition = this.defaultComposition && this.defaultComposition.cloneNode(true);
compositionString = this.defaultComposition;
} else {
// make clone to avoid direct binding
composition = this.stringToDocumentFragment(compositionString);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!compositionString || compositionString !== this._lastLayout || this._forceLayoutChange) {
if (!compositionString) {
composition = this.defaultComposition && this.defaultComposition.cloneNode(true);
} else {
// make clone to avoid direct binding
composition = this.stringToDocumentFragment(compositionString);
}
this.stampComposition(composition);
this._forceLayoutChange = false;
this._lastLayout = compositionString;
Expand All @@ -246,6 +247,7 @@
* @param {Mixed} value New value
*/
StarcounterIncludePrototype._notifyPath = function(path, value) {
var modelAlreadyLoaded = this.template.model == this.partial;
Copy link
Contributor

@alshakero alshakero Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. What about null == null? Maybe even
this.template && this.template.model && this.template.model == this.partial

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point.

if (path.indexOf("partial.") === 0 || path.indexOf("viewModel.") === 0) {
if (
// path === "viewModel.Html" || // covered by viewModel._whatever_
Expand All @@ -259,11 +261,6 @@
(path.indexOf('.Html') === path.length - 5) && path.slice(8, -5).indexOf('.') === -1
) {
this.partialChanged(this.partial);
} else if(
// if viewModel|partial._whatever_, but not _ver - scope changed
path.split('.').length === 2 && path.indexOf('_ver#s') === -1 && path.indexOf('_ver#c$') === -1
){
this.partialChanged(this.partial);
} else {
const compositionProviderPath = findCompositionProviderPath(this.partial);
// if (path === 'partial.' + compositionProviderPath ||
Expand All @@ -280,11 +277,15 @@
} else if (path === 'partial.' + compositionProviderPath + '.PartialId' || path === 'viewModel.' + compositionProviderPath + '.PartialId') {
const compositionProvider = findCompositionProvider(this.partial);
this.partialId = compositionProvider && compositionProvider.PartialId; //should always be string
} else if (this.template._notifyPath) {
this.template._notifyPath(
path.replace("partial.", "model.")
.replace("viewModel.", "model."),
value);
} else {
this.partialChanged(this.partial);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@tomalec tomalec Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@warpech warpech Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@tomalec tomalec Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@warpech warpech Jan 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
Expand Down
11 changes: 11 additions & 0 deletions test/composition/declarative-shadow-dom.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,17 @@ <h2>Custom Shadow DOM composition</h2>
.equal(document.querySelector(REFERENCE_COMPOSITION).innerHTML.trim());
});
});
describe('after partial property is replaced with itself', function () {
it('should keep using the existing composition', function (done) { //otherwise ViewKeeper (aka workspaces) do not keep state in Firefox and Edge
var old = scInclude.shadowRoot.querySelector("h2");
scInclude.partial = scInclude.partial;
setTimeout(function() {
expect(old).to.be.not.null;
expect(old).to.be.equal(scInclude.shadowRoot.querySelector("h2"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

@warpech warpech Jan 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

done();
}, 500);
});
});

});
</script>
Expand Down
25 changes: 23 additions & 2 deletions test/partialAttribute/nested-html/stamped_from_polymer_template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this inconsistent:

  1. the test above (line 96) checks that Polymer notification is forwarded before the template is stamped
  2. 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.

sinon.stub(importedTemplate, "_notifyPath");
var newVal = "notification works";
domBind.set("model.Page.scope1.doesItWork", newVal);
expect(importedTemplate._notifyPath).to.be.calledWith("model.scope1.doesItWork", newVal)
expect(importedTemplate._notifyPath).to.be.not.calledWith("model.scope1.doesItWork", newVal)
});

describe('after `dom-bind`` changed the partial property', function () {
Expand Down Expand Up @@ -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 () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@tomalec tomalec Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var scope;

beforeEach(function(done) {
// IDEA: promisify juicy-html
sinon.stub(importedTemplate, "_notifyPath");
importedTemplate.addEventListener('stamping', function onceStamped(){
importedTemplate.removeEventListener('stamping', onceStamped);
scope = domBind.get('model.Page.scope1');
domBind.set('model.Page.scope1', scope);
done();
});
});

it('should forward the notification to `imported-template`', function () {
expect(importedTemplate._notifyPath).to.be.calledWith("model.scope1", scope)
});
});

describe('after `dom-bind`` changed the sub-partial property (just one scope) to new one with thethat results with the same merged Html', function () {
var changedScopeWithSameHtml;
beforeEach(function(done) {
Expand Down