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

Preact templating engine #71

Closed
dgp1130 opened this issue Mar 11, 2023 · 6 comments
Closed

Preact templating engine #71

dgp1130 opened this issue Mar 11, 2023 · 6 comments
Labels
feature New feature or request
Milestone

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Mar 11, 2023

With the Lit templating engine currently stalled in #70, one possible alternative is Preact. I'm not very familiar with the React / Preact ecosystem, but I think it should be possible to do this.

Apparently Preact includes semi-official support of the htm package which does not use JSX and even looks a lot like lit-html.

I was able to make an initial prototype which mostly works. Got a bit confused as to why <!DOCTYPE html> and <meta charset="utf8"> render so weirdly, but eventually figured out workarounds.

I then tried to include a script, only to discover that Preact (JSX? VDom?) cannot represent comments as there's no way to render them. This is a problem for includeScript() and inlineStyle() which render comments as annotations later processed by the build system.

I'm not immediately able to find a workaround, but I'll have to keep looking. One possible approach would be to change annotations to output actual HTML instead of comments like:

<rules_prerender:include-script path="path/to/pkg/script.js />

This might be useful as I ran into a similar problem when experimenting with SSR, which required nested annotations. Nesting HTML comments is actually kind of a pain in the ass since they are parsed lexically, not grammatically. Using real HTML could solve both problems.

@dgp1130 dgp1130 added the feature New feature or request label Mar 11, 2023
dgp1130 added a commit that referenced this issue Mar 12, 2023
…ag instead of a comment.

Refs #71.

Preact can't easily render comments, meaning it is tricky to support `includeScript()` and `inlineStyle()` with Preact when they use comments. Instead, we render `<rules_prerender:annotation>...</rules_prerender:annotation>`. The `rules_prerender:` technically makes it an XML namespace, though without any explicit declaration. It should be fine since these elements never make it to the browser. Since this is no a full `HTMLElement` and not a `Node`, we can simplify some of the implementation and drop `UpdateableNode`.

For now, I left the actual JSON content in the tag's content. We no longer really need the `RULES_PRERENDER_DO_NOT_DEPEND_OR_ELSE` prefix given that `rules_prerender:annotation` already identifies the relevant tag. I might remove that in a future commit. Later on we might also want to support multiple child nodes for something like SSR. Of course it might be better to just escape the HTML content inside the element.

Most of the changes here are adjusting tests, the actual change in `packages/rules_prerender/{scripts,styles}.mts` is relatively straightforward and just required some updates to `prerender_annotation_walker.mts` to extract the new format.
dgp1130 added a commit that referenced this issue Mar 12, 2023
Refs #71.

This prefix is no longer needed since it is not wrapped in a raw HTML comment, but instead wrapped in a real tag. Now "creating" and "parsing" an annotation is really just calling `JSON.stringify()` and `JSON.parse()`. They don't provide that much utility as abstractions, but ok for now.
dgp1130 added a commit that referenced this issue Mar 12, 2023
Refs #71.

This seems to be compatible with Node 18 AFAICT.
dgp1130 added a commit that referenced this issue Mar 12, 2023
Refs #71.

Under typical options, `tsc` usually generates `*.js` files as the output of `*.tsx` inputs. I think this is configurable, but for now we'll keep it simple and not worry about actual `*.jsx` files. This updates `paths.bzl` to support this new extension.

I also added some missing tests in `paths_test.bzl`, as apparently I missed some recent updates to this file.
dgp1130 added a commit that referenced this issue Mar 12, 2023
Refs #71.

This provides a small wrapper around `rules_prerender` to use Preact as a templating language. For the most part it is a relatively straightforward implementation.

Preact doesn't directly support `<template />` elements, but I was able to add support via a trivial `Template` component. I initially tried to use `htm` but ultimately decided against it because it does not have any validation out of the box right now. It's main benefit is not requiring a compiler, which would be compelling if `tsc` didn't already support standard JSX syntax out of the box.
dgp1130 added a commit that referenced this issue Mar 12, 2023
@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 12, 2023

developit actually responded to some of my complaints around Preact and confirmed that there is no good way to render comments: https://techhub.social/@develwithoutacause/110003437970303438.

I started looking into using <rules_prerender:annotation>{...}</rules_prerender:annotation> and this does appear to be doable. Unfortunately, it will get printed to any .textContent usage, which leaks into user tests. Instead, I tried <template rules_prerender_type="annotation">{...}</template>, however this still surfaces in .textContent because node-html-parser doesn't special case templates like it should. I filed taoqf/node-html-parser#235 to follow up with a proper fix for that. For now, I'll just need to update those tests to be less brittle, but it is unfortunate that these annotations leak into user code like that.

Jason also suggested that the lack of validation in htm is somewhat intentional. JSX is intended to provide a proper compile step with validation, while htm isn't really intended for that use case. Validation could certainly be improved, but it's not as necessary for htm when JSX already provides that as an alternative. Given that JSX is directly supported in TypeScript, I think it's probably better to use that for @rules_prerender examples, though we should be flexible enough for users to plug in their own VDom renderer. I'm not sure if that's the right terminology, but basically users should be able to choose whether or not to use JSX, htm, or anything else for their templates, as long as it is compatible with Preact.

Using <rules_prerender:annotation />, I was able to make an includeScript() and inlineStyle() work. Unfortunately <template /> doesn't work out of the box, but I was able to add support for it via a relatively straightforward <Template /> component.

I tried to move off htm and instead use TSX directly. Unfortunately .mtsx doesn't appear to be a valid extension. We can use *.tsx and it compiles successfully, but outputs a *.js file instead of a *.mjs file. Surprisingly this actually worked, however that is only because it appears to be incorrectly reading the root package.json file outside the sandbox and using it's "type": "module" configuration. Technically I need an explicit data dependency on the root package.json (copied to bin ofc) in order to build this correctly. It's not a terrible workaround, but definitely unintuitive and shouldn't be necessary for every prerender_pages() target.

Regardless I think this is a good enough state to ship in, mostly works with the only major caveat being the file extension issue.

@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 12, 2023

One problem I haven't solved yet is using @rules_prerender/declarative_shadow_dom. polyfillDeclarativeShadowDom() currently returns a string, but AFAICT there's no easy way to parse an HTML string into Preact VDom without another dependency. I'm thinking this could be solved with a import { polyfillDeclarativeShadowDom } from '@rules_prerender/declarative_shadow_dom/preact'; which uses an optional peer dep on @rules_prerender/preact. I'll have to play around with it.

@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 12, 2023

So I tried updating an out-of-repo example I have lying around and ran into an unexpected problem. I can't get TypeScript to type check a custom element in JSX. Apparently you're supposed to extend the JSX.IntrinsicElements interface much like HTMLElementsTagNameMap. But no matter what I try I can't get it to work:

declare global {
    namespace JSX {
        interface IntrinsicElements {
            'my-component': any; // Doesn't have any effect.
        }
    }
}

export function Component({ children }: { children: ComponentChildren }): VNode {
    // Error: Property 'my-component' does not exist on type 'JSX.IntrinsicElements'.
    return <my-component></my-component>;
}

The best workaround I can find is to use createElement('my-component', {}, [ /* ... */), but that definitely shouldn't be necessary. I have no idea what I'm doing wrong here, but I'll have to keep investigating.

@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 13, 2023

So apparently the correct syntax is:

declare module 'preact' {
    namespace JSX {
        interface IntrinsicElements {
            'my-component': {};
        }
    }
}

It feels very hacky to merge into a namespace of a dependency like this, though I'm not sure declare global {} is all that much better, so 🤷.

Playing around with this type, it seems kind of annoying for a lot of components, but also pretty useful for type checking prerender output. If you really want to turn it off, you can do:

declare module 'preact' {
    namespace JSX {
        interface IntrinsicElements {
            [comp: string]: {[prop: string]: unknown};
        }
    }
}

This still supports overriding specific elements with more precise types, so it's actually pretty nice for making things more ergonomic IMHO, though at the cost of strictness. You could easily typo my-compenont and be confused why things don't work at runtime. In fact, in my prototypes, the #1 mistake I make is mistyping the tag name in some capacity, which is annoying to debug because it has no effect on the page, the web component just doesn't execute and no error is surfaced. (Maybe we could fix that with a DevTools extension - https://techhub.social/@develwithoutacause/110012548273306004.)

We can give this a more specific type, but it's a little annoying since the actual "definition" is in the client code, not the prerendering code. Usage of this.getAttribute() implies some type definition on the component's attributes (maybe HydroActive could solve that - filed dgp1130/HydroActive#1 to follow up on it). Unfortunately there's no clear way to emit that as an inferred type. The component itself can define an interface, though it shouldn't depend on preact which is annoying here.

I updated the Template type to properly pass through standard HTML properties. I briefly considered defining a new intrinsic for <template /> in @rules_prerender/preact, but decided against it because that would modify the global types and could be confusing in a Bazel context where not all ts_project() targets will depend on @rules_prerender/preact, so it's only kind of global in an awkward way.

dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

I accidentally used this target today when I actually wanted `//:prerender_components/@rules_prerender/declarative_shadow_dom`. There is not much use for the Node module and using it directly violates the expectation that all the various "slices" of the component stay together in a cohesive way. Limiting visibility makes it easier to catch this mistake in the future.
dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

This adds a new `/preact.mjs` entry point which is implemeted via an optional peer dep on `@rules_prerender/preact`.

I tried adding `exports` to specifically support jsut `/preact.mjs`, but unfortunately that requires `moduleResolution: "Node16"` in `tsconfig.json` which introduces a whole bunch of other problems I don't want to deal with at the moment.
@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 13, 2023

I'm trying to look into updating @rules_prerender/declarative_shadow_dom to expose a /preact path, but this seems to require moduleResolution: "Node16" in TypeScript, which introduces a bunch of module resolution challenges I still don't understand. I wasn't able to find any existing @aspect_rules_ts example which uses Node16, so I'm not sure if this possible or what the problem even is.

Instead I just exposed this functionality in @rules_prerender/declarative_shadow_dom/preact.mjs. There's nothing restricted access to package internals, which I don't like, but this is good enough to solve the problem. I should look into proper exports support in the future.

dgp1130 added a commit that referenced this issue Mar 13, 2023
@dgp1130
Copy link
Owner Author

dgp1130 commented Mar 13, 2023

Released everything in 0.0.26. Next steps are:

  1. Migrate all the internal examples to use Preact (because I want to strongly discourage direct string manipulation).
  2. Add TextResource type which PrerenderResource allows as a string input without sanitization.
  3. Remove string inputs from PrerenderResource in favor of SafeHtml and TextResource.
  4. Remove / discourage any raw string manipulation-based APIs (such as the built-in includeScript() and inlineStyle() as well as the default polyfillDeclarativeShadowDom()).

I don't think those need to be in this issue, so I'll follow up separately with those.

@dgp1130 dgp1130 closed this as completed Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

As much as I like this option for inspectability, it actually changes the output formatting introduces whitespace around elements where it shouldn't. This causes HTML to render the text with additional spaces. Therefore we _must_ disable `pretty` to get the correct rendered output.
dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

This was definitely the most involved migration, though most of it is busy work. Modified and updated tests to be more stable and useful. In particular, I managed to escape the Markdown HTML content yet no tests failed, which should be fixed now.
dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

This move raw string support in `PrerenderResource` to a different function. The main objective is to make it significantly harder to accidentally write a raw string to an HTML file, given that there are little to no safety guarantees. `PrerenderResource.fromText()` accepts a raw string, but refuses to associate it with a `*.html` or `*.htm` file. This supports writing textual files which are _not_ used as HTML content. Instead, users are directed at `PrerenderResource.of()` and `SafeHtml` which should be preferred.

Of course no protection is perfect here. Users could write to a `.definitelyNotAnHtmlFileISwear` and then rename it later in the build process. Though the script and style injection tooling is limted to `*.html` files, so they'd be throwing away a lot of functionality to do that.
dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

This limits `PrerenderResource.of()` to just HTML content, simplifying the implementation and being more clear about the security impact of using different functions. Like `fromText()`, `fromBinary()` also rejects outputting to a `*.html` or `*.htm` file, as `PrerenderResource.of()` should be used instead.
dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

This more accurately reflects its actual usage and semantics now that `fromText()` and `fromBinary()` exist.
dgp1130 added a commit that referenced this issue Mar 13, 2023
Refs #71.

Also fixes a few miscellaneous things like file extensions.
@dgp1130 dgp1130 added this to the 1.0.0 milestone Mar 13, 2023
dgp1130 added a commit that referenced this issue Mar 17, 2023
Refs #71.

The migration is relatively straightforward, with one major bug which needed to be addressed. The comment in `//:node_modules/@rules_prerender/preact` explains most of the context, but essentially we cannot allow `//packages/preact:pkg` to depend on `//:node_modules/rules_prerender` because this is an NPM peer dependency, not a normal dependency. External workspaces cannot `npm_link_package()` from `@rules_prerender//packages/preact:pkg` when there is a transitive dependency on `//:node_modules/rules_prerender` because it duplicates the `rules_prerender` package in addition to the `//:node_modules/rules_prerender` target in their own workspace. This introduces incompatibilities, since user code and `@rules_prerender/preact` would be importing different versions of the `rules_prerender` package.

Instead, we depend on `//:node_modules/rules_prerender_types` which drops the JS implementation from the output to avoid duplicating the `rules_prerender` NPM package. However this alone means there is no dependency whatsoever between `@rules_prerender/preact` and `rules_prerender` and users have to manually depend on both, even when they only import `@rules_prerender/preact`. To address this, we use `deps` in `link_npm_package()` to add the dependency _at link time_. Doing so allows the same package to be linked in multiple places with different implementations of `rules_prerender`. In the `@rules_prerender` workspace, we link it to `@rules_prerender//:node_modules/rules_prerender` as built from HEAD. In other workspaces (such as `examples/external/`), users should link to the `//:node_modules/rules_prerender` of that workspace. This keeps a single definition of `rules_prerender` and always places it in the right location.
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