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

feat: improve HTMLDirective and CSSDirective design for flexibility, consistency, and performance #5799

Closed
EisenbergEffect opened this issue Apr 4, 2022 · 7 comments · Fixed by #5826 or #5835
Assignees
Labels
area:fast-element Pertains to fast-element breaking-change A breaking change to a shipping package closed:done Work is finished task A task that's part of a body of feature work.

Comments

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Apr 4, 2022

HTMLDirective

At present, HTMLDirective has an interface requiring the implementation of both createPlaceholder and createBehavior. The API is awkward, with the placeholder API taking an index as input. This older interface is also different than the CSSDirective interface.

I'd like to modernize this interface, making it more intuitive and powerful. To do this, I propose we change createPlaceholder to createHTML and make that the only required method to implement on HTMLDirective.

The createHTML method would return any HTML it wants inserted into the template. The argument to this method would be a function that can be used to add behavior factories. When adding a behavior factory, the method will return a unique id that can be used in placeholders within the HTML. By separating HTML creation and behavior factories, we'll be able to have directives that contribute arbitrary HTML as well as any number of behaviors to a view.

As an example, AspectedHTMLDirective would be implemented like this:

export abstract class AspectedHTMLDirective extends HTMLDirective {
    // elided

    public createHTML: (add: AddFactory) => string = Markup.interpolation(add(this));
}

In the above example, an interpolation expression is returned to be composed into the template, which contains the unique id of this. this is a directive that also implements the ViewBehaviorFactory interface in this case. While in the past directives had to implement this interface, that is no longer required. A directive under this system would be able to add any number of behaviors including none at all if the directive only wanted to generate HTML.

One way we could leverage this new capability is to make composed views compose at template compilation time instead of at render time. We would do this by making templates inherit from HTMLDirective. Here's what the createHTML method might look like for a template:

createHTML(add: AddFactory) {
    const factories = this.factories;

    for (const key in factories) {
        add(factories[key]);
    }

    return this.html;
}

The template already has internal HTML along with the factories, so it only needs to add those to the new template that's being constructed. Because the new system would leverage unique id's for directives rather than indexes, there won't be any issue of fixing up indexes when merging templates together in this way. In the future we may be able to teach the template compiler how to reuse previously compiled template pieces as well.

CSSDirective

We can make a similar set of changes to CSSDirective, making the overall system more consistent and symmetrical. The createCSS function would take a similar add function to enable adding of behaviors. We could then optimize CSSPartial so that it only adds behaviors if there are internal style sheets or behaviors. Simple CSS partials would not need to pay the performance cost anymore.

@EisenbergEffect EisenbergEffect moved this from Triage to Todo in FAST Architecture Roadmap Apr 4, 2022
@EisenbergEffect EisenbergEffect self-assigned this Apr 4, 2022
@EisenbergEffect EisenbergEffect added this to the FASTElement 2.0 milestone Apr 4, 2022
@EisenbergEffect EisenbergEffect added area:fast-element Pertains to fast-element breaking-change A breaking change to a shipping package task A task that's part of a body of feature work. status:planned Work is planned labels Apr 4, 2022
@EisenbergEffect EisenbergEffect changed the title feat: improve HTMLDirective design for flexibility and consistency feat: improve HTMLDirective and CSSDirective design for flexibility, consistency, and performance Apr 4, 2022
@EisenbergEffect EisenbergEffect moved this from Todo to In Progress in FAST Architecture Roadmap Apr 5, 2022
@rajsite
Copy link

rajsite commented Apr 6, 2022

It would also be nice if the tag template helpers html and css could be used standalone. We use html standalone heavily for writing storybook stories with a helper like the following:

const fragment = document.createDocumentFragment();
viewTemplate.render(source, fragment);
const dummyElement = document.createElement('div');
dummyElement.append(fragment);
const result = dummyElement.innerHTML;

The resulting HTML is a bit messy though, has extra comments inserted. Elements have come alive and done things like push classes onto themselves that are observable, etc.

It would be nice to be able to use the html template helpers as a standalone tool for generating HTML strings (things like bindings, etc being ignored or erroring if used).

Also related, it would be helpful if DesignTokens rendered as the CSS custom property string when used in html templates like they do in css templates:

const template = html`
    <style>
        .my-label {
            color: ${myColorDesignToken};
        }
    </style>
`;

@EisenbergEffect
Copy link
Contributor Author

The new SSR renderer we're working on returns a string, and handles all directives, bindings, etc. Would that be of interest to you? You would still need a helper function probably, but for use cases like this it would probably be pretty straight forward.

I'll think about the design tokens in html scenario. We might need a new way to detect directives other than instanceof since you can't both inherit from HTMLDirective and CSSDirective at once. I'm working on this now so I'll think through that a bit.

@rajsite
Copy link

rajsite commented Apr 6, 2022

The new SSR renderer we're working on returns a string, and handles all directives, bindings, etc. Would that be of interest to you?

Potentially? I'd imagine the SSR renderer would be trying to emit the full instantiated template of fast elements as declarative shadow dom with style, etc.

The use-case I'm thinking of wouldn't have that instantiation, ie

html`
<my-ul>
    ${repeat(x=>x.items, html`
        <my-li>${y => y.value}</my-li>
    `)}
</my-ul>`.renderAsString({items: [{value: 'a'}, {value: 'b'}]})

Would just result in the string:

<my-ul><my-li>a</my-li><my-li>b</my-li></my-ul>

@EisenbergEffect
Copy link
Contributor Author

What if there was an option not to render the declarative shadow dom?

@rajsite
Copy link

rajsite commented Apr 6, 2022

What if there was an option not to render the declarative shadow dom?

Sounds very likely to be useful! Only other thing I can think of is it not being coupled to node.js apis, etc as this case would effectively be client-side SSR :)

@EisenbergEffect
Copy link
Contributor Author

Ah. I don't think it's coupled to node.js. @nicholasrice as been doing the work so he can share more.

If it is client-side specifically, it might be better to just have some helpers for the existing client renderer instead. I need to think through it a bit. We do have some stuff like this in our tests. I've been thinking about exposing some test stuff from foundation, so maybe this just becomes part of that and gets cleaned up?

@EisenbergEffect EisenbergEffect moved this from In Progress to In Review in FAST Architecture Roadmap Apr 11, 2022
@EisenbergEffect EisenbergEffect linked a pull request Apr 11, 2022 that will close this issue
5 tasks
@EisenbergEffect EisenbergEffect linked a pull request Apr 13, 2022 that will close this issue
5 tasks
@EisenbergEffect EisenbergEffect added closed:done Work is finished and removed status:planned Work is planned labels Apr 18, 2022
Repository owner moved this from In Review to Done in FAST Architecture Roadmap Apr 18, 2022
@nicholasrice
Copy link
Contributor

Hmm I haven't put effort into testing if this runs outside NodeJS. The SSR code itself isn't leveraging any NodeJS APIs but it does take one dependency (an HTML parser), and I'm unsure if it hits any NodeJS apis or not. I suppose we could take a task to investigate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fast-element Pertains to fast-element breaking-change A breaking change to a shipping package closed:done Work is finished task A task that's part of a body of feature work.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants