Skip to content

Commit

Permalink
fix(fast-element): do not notify when binding observer is disconnected (
Browse files Browse the repository at this point in the history 
microsoft#3418)

* test(fast-element): ensure basic composition tests include bindings

* fix(fast-element): do not notify when binding observer is disconnected
  • Loading branch information
@EisenbergEffect
EisenbergEffect authored Jun 30, 2020
1 parent 34742de commit dbf3093
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 27 deletions.
49 changes: 24 additions & 25 deletions packages/web-components/fast-element/src/directives/binding.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ describe("The binding directive", () => {

@observable value: any = null;
@observable private trigger = 0;
@observable knownValue = "value";

forceComputedUpdate() {
this.trigger++;
Expand Down Expand Up @@ -67,22 +68,22 @@ describe("The binding directive", () => {
context("when binding template content", () => {
it("initially inserts a view based on the template", () => {
const { behavior, parentNode } = contentBinding();
const template = html`This is a template.`;
const template = html<Model>`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);
});

it("removes an inserted view when the value changes to plain text", async () => {
const { behavior, parentNode } = contentBinding();
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

model.value = "This is a test.";

Expand All @@ -93,12 +94,12 @@ describe("The binding directive", () => {

it("removes an inserted view when the value changes to null", async () => {
const { behavior, parentNode } = contentBinding();
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

model.value = null;

Expand All @@ -109,12 +110,12 @@ describe("The binding directive", () => {

it("removes an inserted view when the value changes to undefined", async () => {
const { behavior, parentNode } = contentBinding();
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

model.value = void 0;

Expand All @@ -125,26 +126,24 @@ describe("The binding directive", () => {

it("updates an inserted view when the value changes to a new template", async () => {
const { behavior, parentNode } = contentBinding();
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

const newTemplate = html`This is a new template, different from before.`;
const newTemplate = html<Model>`This is a new template ${x => x.knownValue}`;
model.value = newTemplate;

await DOM.nextUpdate();

expect(toHTML(parentNode)).to.equal(
`This is a new template, different from before.`
);
expect(toHTML(parentNode)).to.equal(`This is a new template value`);
});

it("reuses a previous view when the value changes back from a string", async () => {
const { behavior, parentNode, node } = contentBinding();
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);
Expand All @@ -154,7 +153,7 @@ describe("The binding directive", () => {

expect(view).to.be.instanceOf(HTMLView);
expect(capturedTemplate).to.equal(template);
expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

model.value = "This is a test string.";

Expand All @@ -171,38 +170,38 @@ describe("The binding directive", () => {

expect(newView).to.equal(view);
expect(newCapturedTemplate).to.equal(capturedTemplate);
expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);
});

it("doesn't compose an already composed view", async () => {
const { behavior, parentNode } = contentBinding("computedValue");
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

model.value = template;
model.forceComputedUpdate();

await DOM.nextUpdate();

expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);
});
});

context("when unbinding template content", () => {
it("unbinds a composed view", () => {
const { behavior, node, parentNode } = contentBinding();
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

const newView = (node as any).$fastView as SyntheticView;
expect(newView.source).to.equal(model);
expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

behavior.unbind();

Expand All @@ -211,14 +210,14 @@ describe("The binding directive", () => {

it("rebinds a previously unbound composed view", () => {
const { behavior, node, parentNode } = contentBinding();
const template = html`This is a template.`;
const template = html`This is a template. ${x => x.knownValue}`;
const model = new Model(template);

behavior.bind(model, defaultExecutionContext);

const view = (node as any).$fastView as SyntheticView;
expect(view.source).to.equal(model);
expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);

behavior.unbind();

Expand All @@ -228,7 +227,7 @@ describe("The binding directive", () => {

const newView = (node as any).$fastView as SyntheticView;
expect(newView.source).to.equal(model);
expect(toHTML(parentNode)).to.equal(`This is a template.`);
expect(toHTML(parentNode)).to.equal(`This is a template. value`);
});
});
});
23 changes: 23 additions & 0 deletions packages/web-components/fast-element/src/observation/observable.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -541,5 +541,28 @@ describe("The Observable", () => {
value = observer.observe(model, defaultExecutionContext);
expect(value).to.equal(binding(model));
});

it("does not notify if disconnected", async () => {
let wasCalled = false;
const binding = (x: Model) => x.value;
const observer = Observable.binding(binding, {
handleChange() {
wasCalled = true;
},
});

const model = new Model();

const value = observer.observe(model, defaultExecutionContext);
expect(value).to.equal(model.value);
expect(wasCalled).to.equal(false);

model.value++;
observer.disconnect();

await DOM.nextUpdate();

expect(wasCalled).to.equal(false);
});
});
});
6 changes: 4 additions & 2 deletions packages/web-components/fast-element/src/observation/observable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,9 @@ class BindingObserverImplementation<TSource = any, TReturn = any, TParent = any>

/** @internal */
call(): void {
this.needsQueue = true;
this.notify(this);
if (this.last !== null) {
this.needsQueue = true;
this.notify(this);
}
}
}

0 comments on commit dbf3093

Please sign in to comment.