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

Fix noted TODOs #2072

Merged
merged 6 commits into from
Aug 27, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/heavy-melons-walk.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'lit-element': patch
'lit-html': patch
'@lit/reactive-element': patch
---

Remove unneeded `matches` support in @queryAssignedNodes. Update styling tests to use static bindings where needed. Fix TODOs related to doc links.
3 changes: 1 addition & 2 deletions packages/lit-element/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ export * from 'lit-html';
export * from './lit-element.js';
export * from './decorators.js';

// TODO: link to docs on the new site
console.warn(
"The main 'lit-element' module entrypoint is deprecated. Please update " +
"your imports to use the 'lit' package: 'lit' and 'lit/decorators.ts' " +
"or import from 'lit-element/lit-element.ts'."
"or import from 'lit-element/lit-element.ts'. See https://lit.dev/docs/releases/upgrade/#update-packages-and-import-paths for more information."
);
6 changes: 3 additions & 3 deletions packages/lit-element/src/lit-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ if (DEV_MODE) {
if (obj[name] !== undefined) {
console.warn(
`\`${name}\` is implemented. It ` +
`has been removed from this version of LitElement. `
// TODO(sorvell): add link to changelog when location has stabilized.
// + See the changelog at https://github.com/lit/lit/blob/main/packages/lit-element/CHANGELOG.md`
`has been removed from this version of LitElement. See ` +
`https://lit.dev/docs/releases/upgrade/#litelement ` +
`for more information.`
);
}
};
Expand Down
12 changes: 6 additions & 6 deletions packages/lit-element/src/test/lit-element_styling_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* SPDX-License-Identifier: BSD-3-Clause
*/
import {css, html as htmlWithStyles, LitElement} from '../lit-element.js';
import {html as staticHtml, unsafeStatic} from 'lit-html/static.js';

import {
canTestLitElement,
Expand Down Expand Up @@ -334,19 +335,17 @@ import {assert} from '@esm-bundle/chai';
}
});

// TODO(sorvell): Bindings in styles are no longer supported.
// This will be supported via static bindings only.
test.skip('properties in styles render with initial value and cannot be changed', async () => {
test('static properties in styles render with initial value and cannot be changed', async () => {
let border = `6px solid blue`;
const name = generateElementName();
customElements.define(
name,
class extends LitElement {
render() {
return htmlWithStyles`
return staticHtml`
<style>
div {
border: ${border};
border: ${unsafeStatic(border)};
}
</style>
<div>Testing...</div>`;
Expand All @@ -356,14 +355,15 @@ import {assert} from '@esm-bundle/chai';
const el = document.createElement(name) as LitElement;
container.appendChild(el);
await el.updateComplete;
const div = el.shadowRoot!.querySelector('div');
let div = el.shadowRoot!.querySelector('div');
assert.equal(
getComputedStyleValue(div!, 'border-top-width').trim(),
'6px'
);
border = `4px solid orange`;
el.requestUpdate();
await el.updateComplete;
div = el.shadowRoot!.querySelector('div');
assert.equal(
getComputedStyleValue(div!, 'border-top-width').trim(),
'6px'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {repeat} from '../../directives/repeat.js';
import {cache} from '../../directives/cache.js';
import {assert} from '@esm-bundle/chai';
import {renderShadowRoot, wrap, shadowRoot} from '../test-utils/shadow-root.js';
import {html as staticHtml, unsafeStatic} from '../../static.js';

import '../lit-html_test.js';
// selected directive tests
Expand Down Expand Up @@ -387,32 +388,32 @@ suite('polyfill-support rendering', () => {
wrap(document.body).removeChild(container2);
});

// TODO(sorvell): This will only be supported via static bindings.
test.skip('part values render into styles once per scope', function () {
test('static part values render into styles once per scope', function () {
if (typeof window.ShadyDOM === 'undefined' || !window.ShadyDOM.inUse) {
this.skip();
return;
}
const container = document.createElement('scope-3');
wrap(document.body).appendChild(container);
const renderTemplate = (border: string) => {
const result = html`
const result = staticHtml`
<style>
div {
border: ${border};
border: ${unsafeStatic(border)};
}
</style>
<div>Testing...</div>
`;
renderShadowRoot(result, container);
};
renderTemplate('1px solid black');
const div = shadowRoot(container)!.querySelector('div');
let div = shadowRoot(container)!.querySelector('div');
assert.equal(
getComputedStyle(div!).getPropertyValue('border-top-width').trim(),
'1px'
);
renderTemplate('2px solid black');
div = shadowRoot(container)!.querySelector('div');
assert.equal(
getComputedStyle(div!).getPropertyValue('border-top-width').trim(),
'1px'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,6 @@
import {ReactiveElement} from '../reactive-element.js';
import {decorateProperty} from './base.js';

// TODO(sorvell): Remove when https://github.com/webcomponents/polyfills/issues/397 is addressed.
// x-browser support for matches
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const ElementProto = Element.prototype as any;
const legacyMatches =
ElementProto.msMatchesSelector || ElementProto.webkitMatchesSelector;
Copy link
Member

Choose a reason for hiding this comment

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

tests:  ❌ @queryAssignedNodes > returns assignedNodes for slot that contains text nodes filtered by selector when Element.matches does not exist
tests:       TypeError: node.matches is not a function
tests:         at ../reactive-element/src/decorators/query-assigned-nodes.ts:60:32
tests:         at Array.filter (<anonymous>)
tests:         at HTMLElement.get [as footerAssignedItems] (../reactive-element/src/decorators/query-assigned-nodes.ts:57:24)
tests:         at o.<anonymous> (../reactive-element/src/test/decorators/queryAssignedNodes_test.ts:221:40)

😭


/**
* A property decorator that converts a class property into a getter that
* returns the `assignedNodes` of the given named `slot`. Note, the type of
Expand Down Expand Up @@ -64,10 +57,7 @@ export function queryAssignedNodes(
nodes = nodes.filter(
(node) =>
node.nodeType === Node.ELEMENT_NODE &&
// eslint-disable-next-line @typescript-eslint/no-explicit-any
((node as any).matches
? (node as Element).matches(selector)
: legacyMatches.call(node as Element, selector))
(node as Element).matches(selector)
);
}
return nodes;
Expand Down
6 changes: 4 additions & 2 deletions packages/reactive-element/src/reactive-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,6 @@ export abstract class ReactiveElement
}
);
if (shadowedProperties.length) {
// TODO(sorvell): Link to docs explanation of this issue.
console.warn(
`The following properties will not trigger updates as expected ` +
`because they are set using class fields: ` +
Expand All @@ -1063,7 +1062,10 @@ export abstract class ReactiveElement
`accessors used for detecting changes. To fix this issue, ` +
`either initialize properties in the constructor or adjust ` +
`your compiler settings; for example, for TypeScript set ` +
`\`useDefineForClassFields: false\` in your \`tsconfig.json\`.`
`\`useDefineForClassFields: false\` in your \`tsconfig.json\`.` +
`See https://lit.dev/docs/components/properties/#declare and ` +
`https://lit.dev/docs/components/decorators/` +
`#avoiding-issues-with-class-fields for more information.`
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,15 +198,7 @@ const flush =
assert.deepEqual(el.assignedNodesEl.footerAssignedItems, []);
});

test('returns assignedNodes for slot that contains text nodes filtered by selector when Element.matches does not exist', () => {
const descriptor = Object.getOwnPropertyDescriptor(
Element.prototype,
'matches'
);
Object.defineProperty(Element.prototype, 'matches', {
value: undefined,
configurable: true,
});
test('returns assignedNodes for slot that contains text nodes filtered by selector', () => {
assert.deepEqual(el.assignedNodesEl.footerAssignedItems, []);
const child1 = document.createElement('div');
const child2 = document.createElement('div');
Expand All @@ -222,9 +214,6 @@ const flush =
el.removeChild(child2);
flush();
assert.deepEqual(el.assignedNodesEl.footerAssignedItems, []);
if (descriptor !== undefined) {
Object.defineProperty(Element.prototype, 'matches', descriptor);
}
});

test('always returns an array, even if the slot is not rendered', () => {
Expand Down