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 UnsafeStatic extension #568

Closed
wants to merge 2 commits into from
Closed

Conversation

Lodin
Copy link

@Lodin Lodin commented Oct 13, 2018

Fixes #78
Fixes #218
Closes #274

One of the things I always missed in lit-html was the inability to use variables as tag names. Especially it affects work with WebComponents and their string names. You just unable to check if you've done everything correctly at compile time. If you write component name in a wrong way or if you don't import proper file you can know it only at runtime. And even in runtime you don't have a proper error support because component with a wrong name will just be an empty tag.

The initial implmenetation of that functional, #274, has its own performance issues mostly connected with the fact that unsafe static values breaks the basic flow of the lit-html core.

The latest discussion about moving Promises support out of the core (#550) was the last push to create this PR. I believe that we really don't need to have everything inside the core. Core should be fast and simple, and additional functional can be implemented anywhere else. E.g. not everyone want to use static values, they would be good with a vanilla html function. That's why it is important to let them avoid loading additional bytes.

Basing on this idea I suggest a conception of "extensions", a simple decorator functions that wrap the original html and add new abilities to it. The withUnsafeStatic function proposed by this PR provides merging static values into the template and caching the result to a WeakMap. Each following repetition uses the cache and removes all UnsafeStatic values from values list.

Also there was a blocker I didn't aware of, but looks like it is already resolved: chakra-core/ChakraCore#5201.

Example

Basic usage of unsafe static values is following. However, since withUnsafeStatic doesn't sanitize and validate incoming values it wouldn't be safe to use them this way.

// utils.ts
import {html} from 'lit-html';
import {withUnsafeStatic} from 'lit-html/lib/extensions/unsafe-static';

export const shtml = withUnsafeStatic(html);

// my-component.ts
import {shtml} from './utils'; 

export default class MyComponent extends LitElement {
  public static readonly is: string = name;

  public render() {
    return shtml`<div><slot/></div>`
  }
}

// app.ts
import {unsafeStatic} from 'lit-html/lib/extensions/unsafe-static';
import {MyComponent as MyComponentClass} from './my-component';
import {shtml} from './utils'; 

const MyComponent = unsafeStatic(MyComponentClass.is);

export default class App extends ListElement {
  public render() {
    return shtml`<${MyComponent}>Content</${MyComponent}>`
  }
}

@justinfagnani
Copy link
Collaborator

Hi @Lodin. Thanks for the PR. I'll take a look soon, but this is a relatively significant new pattern that'd we'd be introducing, and I'd like to discuss these thing in issue first before reviewing code. #78 is a good place for that.

While I do want to explore the idea of customizing the html tag further, I want to do it in a deliberate way that addresses multiple uses cases.

Also, while Chakra has merged the fix for the template literal identity bug, I don't know if it made it to Edge 18 or not, and we probably need it released to the last two versions of Edge before we start relying on it.

@daKmoR
Copy link
Contributor

daKmoR commented Nov 3, 2018

hey,
Lodin that really sounds nice - would you mind publishing it independently from lit-html?

as for certain use cases where performance is not the primary goal this would already be a viable solution (until it's solved within lit-html itself). e.g. things like testing or component demos could already benefit from it.

@Lodin
Copy link
Author

Lodin commented Nov 8, 2018

@daKmoR, thanks for the response!

About publishing a package. I'm not sure it is a good idea to publish this package separately because it will be deprecated as soon as unsafe statics are implemented in lit-html. I would prefer to use this code directly and wait until this PR or similar solution is merged.

Regarding performance. Not sure I understand your concerns about it. This implementation should work as fast as lit-html itself except for the initial start where all computations are done. I'm not sure about Edge though since I still cannot get the 18 version but it is a bug and it will be resolved eventually.

@motss
Copy link

motss commented Jan 19, 2019

Does it support SVG or just HTML?

@Lodin
Copy link
Author

Lodin commented Jan 19, 2019

@motss, well, basically it supports anything that default html supports. Could you describe your case?

@motss
Copy link

motss commented Jan 19, 2019

@Lodin I see. I'm thinking about extending such unsafe static thing to SVG also. So far there are 2 things that concern me, one is static Markdown content, another is static SVG icons. I think the static Markdown content is quite similar to the basic example you provided, so I'll just talk about the SVG use case.

From the following basic example, a simple menu button with a SVG icon. Because of the way the project is structured, icons are in another file. menuIcon is imported and it is expected to be always static once rendered (a menu button is going to have a menuIcon and it will not change).

import { menuIcon } from './app-icons.js';
...

render() {
  return html`
    <div class="header">
      <button class="menu-button" aria-label="menu">${menuIcon}</button>
    </div>
  `;
}
...

Since there is already a unsafeHTML (and soon a unsafeStatic), I'm thinking whether a unsafeSVG or unsafeStatic for SVG makes sense or not.

@Lodin
Copy link
Author

Lodin commented Jan 19, 2019

@motss, hmm, am I getting it correctly: you don't use svg or html of lit-html for menuIcon? If it is just a string, than yes, you can use unsafeStatic here. If it is a TemplateResult, than you cannot do it, because TemplateResult is not static by definition (it can be used "as is" though, because engine just won't update your value here).

@motss
Copy link

motss commented Jan 19, 2019

@Lodin I see. Now I get it. The important bit is that the input has to be of type string.

So the revised code will be the following based on my understanding.

...
const staticMenuIcon = unsafeStatic('<svg>...</svg>'); // This will work but not html`<svg>...</svg>`;
...
render() {
  return html`
    <div class="header">
      <button class="menu-button" aria-label="menu">${staticMenuIcon}</button>
    </div>
  `;
}

@Lodin
Copy link
Author

Lodin commented Jan 19, 2019

@motss, correct. Also don't forget that in accordance with this PR (there may also be another implementation since this PR is just a suggestion) html should be a product of withUnsafeStatic function:

import {html} from 'lit-html';
import {unsafeStatic, withUnsafeStatic} from 'lit-html/lib/extensions/unsafeStatic';

const shtml = withUnsafeStatic(html);

...
const staticMenuIcon = unsafeStatic('<svg>...</svg>'); // This will work but not html`<svg>...</svg>`;
...
render() {
  return shtml`
    <div class="header">
      <button class="menu-button" aria-label="menu">${staticMenuIcon}</button>
    </div>
  `;
}

@motss
Copy link

motss commented Jan 19, 2019

@Lodin Cool. Thanks for the explanation.

@justinfagnani
Copy link
Collaborator

Closing, since we have static support in lit-html 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants