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.

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

Using a directive in an attribute causes it to be marked dirty even if the final value is unchanged #890

Closed
6 tasks done
tec27 opened this issue Apr 25, 2019 · 2 comments · Fixed by #1064
Closed
6 tasks done

Comments

@tec27
Copy link

tec27 commented Apr 25, 2019

Description

The mechanism by which directives are "resolved" in AttributePart results in it erroneously marking a value as dirty as long as there is at least one level of directive "between" a template and the final output. This means that any attribute that is set to a directive is effectively always marked dirty and re-applied on every render.

Steps to Reproduce

  1. Write this code
import {html, render} from 'lit-html';
import {ifDefined} from 'lit-html/directives/if-defined';

function maybeWithMaxlength(haveMaxlength) {
  return html`<input id="my-text"
      type="text"
      maxlength="${ifDefined(haveMaxlength ? 20 : undefined)}" />`;
}

render(maybeWithMaxlength(true), document.body);

const input = document.querySelector('#my-text');
const observer = new MutationObserver(mutations => {
  alert('maxlength mutated!');
});
observer.observe(input, { attributes: true, attributeFilter: ['maxlength'] });

render(maybeWithMaxlength(true), document.body);

Live Reproduction Link

https://stackblitz.com/edit/lit-html-thtgvg?file=src/index.js

Expected Results

No alert would be shown (maxlength would not be mutated the second time, as its underlying value hasn't actually been changed)

Actual Results

Alert is shown.

Browsers Affected

  • Chrome
  • Firefox
  • Edge
  • Safari 11
  • Safari 10
  • IE 11

Additional Notes

This looks to me to be a problem with AttributePart#setValue, here: https://github.com/Polymer/lit-html/blob/master/src/lib/parts.ts#L113

When a directive is set as the value, the template that contains the attribute calls AttributePart.setValue with the directive (thus changing this.value from the previous value of 20 to a reference to the directive function). When the directive function is later resolved as part of the commit process, setValue is called again with the result of the directive (still 20), but since this.value is currently the directive and 20 is a primitive that isn't equal to the directive, the AttributePart gets marked as dirty.

@ilhan007
Copy link

ilhan007 commented May 21, 2019

Recently, I experienced the same issue. I have a custom directive and although the value of an attribute (the result of the directive) does not change, setAttribute is called by lit-html with the same value every time the template re-renders.

The case I hit is with the img tag, if you use a directive (reproduces with the built-in ifDefined as well) with the "src", calling setAttribute on each rendering causes network request for the image (if cache is not present or cache expires).

<img src="${myDirective(src)}"

This is a quick hack, that seems to workaround the issue, but it must be a better way of fixing it. The directive below is the built-in ifDefined directive with few additional lines, that can be seen below. It checks if the value is changed and if not - sets "noChange".

export const ifDefined = directive((value: unknown) => (part: Part) => {
	if (value === undefined && part instanceof AttributePart) {
		if (value !== part.value) {
			const name = committer.name;
			element.removeAttribute(name);
		}
	} else {
+		if (part.committer && part.committer.element && part.committer.element.getAttribute(part.committer.name) === value) {
+			part.setValue(noChange);
+		} else {
			part.setValue(value);
+		}
	}
});

@justinfagnani
Copy link
Collaborator

This is a general gotcha with directives, but I'm not sure yet how to address this automatically in AttributePart, so the fix in ifDefined seems like a good intermediate fix. I'll make a PR.

@justinfagnani justinfagnani added this to the lit-html 1.2.0 milestone Feb 11, 2020
sorvell pushed a commit that referenced this issue Sep 1, 2020
Non-breaking fixes for minor issues.

* fixes lit-element/#722
* fixes lit-element/#890
sorvell pushed a commit that referenced this issue Sep 2, 2020
Non-breaking fixes for minor issues.

* fixes lit-element/#722
* fixes lit-element/#890
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants