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

Lit templating engine #70

Open
dgp1130 opened this issue Mar 5, 2023 · 3 comments
Open

Lit templating engine #70

dgp1130 opened this issue Mar 5, 2023 · 3 comments
Labels
feature New feature or request

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Mar 5, 2023

To improve security of the rendered output, we should not be using raw strings. Instead we should be using a real templating system which escapes data and input from the actual structure of the rendered HTML. This isn't a massive concern in a Bazel context for rendering at build time, since Bazel requires builds to be hermetic and checked in source code is usually pretty trusted. However potential supply chain vulnerabilities could leak malicious content into their outputs which get sent to users, so this is still good to do. If we move forward with SSR, this will be even more important.

For now, I'd like to start with lit-html as a templating engine because:

  1. It nicely fits the mental model of @rules_prerender.
  2. It works in the browser and in Node, so templates can be shared between client side rendering and prerendering.
    • No need for DOM emulation in Node either, as long as we limit our usage to lit-html and don't pull in LitElement.
  3. It doesn't require a compile-step, which means we don't need a special compiler plugin.
    • A compile-step wouldn't be a hard-blocker given that we have a very comprehensive build system available, but prerender_component() directly wraps the prerender ts_project() or js_library(), meaning it is very convenient (though not strictly required) to have first-party support for any templating engine with a compile-step.
  4. It aligns with JavaScript syntax (no change in the language syntax or grammar).
  5. I'm somewhat familiar with it already.

I'm also open to considering a JSX-based templating system since that has direct support in tsc. I'm not very familiar with it though, so I think that should come secondary to a lit-html engine.

@dgp1130 dgp1130 added the feature New feature or request label Mar 5, 2023
@dgp1130 dgp1130 added this to the 1.0.0 milestone Mar 5, 2023
@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 8, 2023

I've made some good progress on this so far, but ran into an issue attempting to render declarative shadow DOM with lit-html. Apparently rendering expressions inside <template /> contents is not allowed (https://lit.dev/docs/templates/expressions/#invalid-locations). I'm not sure exactly why, but would guess it might have to do with the fact that <template /> tags don't necessarily have to contain HTML and could contain any content. There was an attempt to add support for this but was rejected for adding too much bundle size overhead (lit/lit#1759). Instead, we're supposed to use "static expressions". I don't think I fully understand how those work, but it seems like they are intended to allow modification or reuse of template parts which normally aren't allowed to be dynamic (such as a tag name). Static expressions can be used to render inside <template /> elements normally, how this is broken in @lit-labs/ssr (see lit/lit#2246). Comments imply that this is doable, but introduces some complexity around hydration.

A workaround of adding ${null} to the end of templates is suggested, but this only sometimes seems to work.

Even if this issue gets fixed, it does introduce some non-trivial complexity for @rules_prerender. I haven't looked into using Lit's hydration, since that's not really the way I want to go about this. But @rules_prerender directly authors declarative shadow DOM in templates quite a lot. This means that ~all expressions will be inside a <template /> and require the static version of the html symbol as well as literal and other static functions. I don't fully understand the performance implications of using static expressions for everything. Given that in a prerendering context there is no reactivity I don't think it would have a huge impact. Though if any templates are shared with the client, they might need to be static as well, which could hurt client-side performance for no good reason.

I also wonder if the restrictions on <template /> actually applies for the DSD case, but someone from Lit will have to chime in there. Given the way that DSD is used in @rules_prerender, I'm not sure how to move forward here.

@dgp1130 dgp1130 removed this from the 1.0.0 milestone Mar 13, 2023
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 22, 2023

Rendering <template> elements might be fixed now, need to give this another shot: lit/lit#2246.

@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 31, 2023

I upgraded the Lit dependency and rebased the branch to try again. Unfortunately I still get the same error. I filed an issue more directly about this use case to see if we can find a fix and if DSD templates actually need to use static expressions.

lit/lit#4058

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant