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

Add @query, @queryAll and @customElement decorators #159

Merged
merged 2 commits into from
Sep 12, 2018
Merged

Add @query, @queryAll and @customElement decorators #159

merged 2 commits into from
Sep 12, 2018

Conversation

justinfagnani
Copy link
Contributor

Fixes #158

@MaKleSoft
Copy link

MaKleSoft commented Aug 29, 2018

Would it be worthwhile to cache the result of the querySelector call? One could opt in or out of caching via a second decorator argument.

Also, instead of @customElement why not just use @element? 🚲🏠

function _query<T>(queryFn: (target: NodeSelector, selector: string) => T) {
return (selector: string) => (proto: any, propName: string) => {
Object.defineProperty(proto, propName, {
get(this: HTMLElement) { return queryFn(this.shadowRoot!, selector); },

Choose a reason for hiding this comment

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

Just wondering, but shouldn't this be this.renderRoot instead of this.shadowRoot in case someone overrides createRenderRoot() to return something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah...

*
* @param tagName The name of the custom element to define.
*/
export const customElement = (tagName: string) => (clazz: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered how this will extend to scoped custom element registries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not very thoroughly, but this is basically the same as a customElements.define() statement in the module. That will define on the global registry, and redefinitions of the tag name in scoped registries should be ok.

@sorvell sorvell added this to the 0.6.0 milestone Sep 11, 2018
/**
* Class decorator factory that defines the decorated class as a custom element.
*
* @param tagName The name of the custom element to define.
Copy link
Member

Choose a reason for hiding this comment

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

Remove capitalization and period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

(clazz: Constructor<HTMLElement>) => {
window.customElements.define(tagName, clazz);
// Cast as any
return clazz as any;
Copy link
Member

Choose a reason for hiding this comment

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

Why the any cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

test('returns an element when it exists', async () => {
const c = new C();
container.appendChild(c);
await Promise.resolve();
Copy link
Member

Choose a reason for hiding this comment

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

await c.updateComplete...
+ a few other places 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.

done


suite('@customElement', () => {
test('defines an element', () => {
@customElement('lit-element-test-1' as keyof HTMLElementTagNameMap)
Copy link
Member

Choose a reason for hiding this comment

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

Use generateElementName to ensure uniqueness of element name... in all tests that define elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@justinfagnani
Copy link
Contributor Author

@sorvell PTAL

@sorvell sorvell merged commit 021cf8a into master Sep 12, 2018
@sorvell sorvell deleted the query branch September 12, 2018 22:14
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 this pull request may close these issues.

5 participants