Client script and style-only dependencies drop related resources

[dgp1130]

On mobile, so I'll need to elaborate more later. But I'm pretty sure that when a prerender_component() depends on another prerender_component() but only uses client side scripts or styles (no prerender logic) then those client side files would be deemed unused and tree shaken out of the bundle.

I have some thoughts about how to address this, but it involves a pretty significant rearchitecture and mild abuse of aspect(). On the bright side it may reduce the responsibilities of prerender_component() whose broad scope has been bothering me for a long time now.

[dgp1130]

To elaborate a little more, prerender_component() is supposed to tie together all of the prerendered HTML, client-side JavaScript, CSS, and static file resources. However, if you only have a dependency on client-side JS, the CSS and resources won't be processed by prerender_pages() and won't be in the resulting bundle. Even if we solve that problem, we still expect prerendered HTML to call includeScript() or includeStyle(). If neither of those are called, then the client JS and CSS will be tree shaken out of the bundle. You could manually work around this by having a render() function which just calls includeScript() and includeStyle(), but that's an awkward workaround and it requires every user of that client-side rendered component to a have prerender dependency on it, just to include the scripts and styles.

The problem here is that prerender_component()'s deps attribute is only meaningful for the prerender NodeJS code. It should also apply to client side JS, CSS, and even static file resources (which can have semantic dependencies on each other). We can't allow client-side JS to depend on another component's client-side JS because that would drop the CSS and static files it might require. If there is a client-side dependency on a web component that is never prerendered, we should include the CSS and static files in the final bundle. We don't yet have a good story for a component that is client-side rendered, as CSS would need to be manually included and is likely to be duplicated with the prerendered version if present.

prerender_component() already generates re-exports of client-side JS, CSS, and resources, but users are not supposed to depend on them directly, instead another prerender_component() is the only supported usage. This means we have no way of expressing a "client-side dependency" between two components. The obvious solution is to update prerender_component() to have a separate client_deps field that accepts other prerender_component() targets, but this scales poorly as we include CSS and resources. This macro is already way too complicated and I don't want to add more responsibility to it.

I have one thought for how this might work. We could make the client-side JS, CSS, and resources public and make users depend on those re-exports directly. This way, a user would write their BUILD.bazel file like this:

load("@npm//rules_prerender:index.bzl", "prerender_component", "prerender_resources")
load("@npm//@bazel/typescript:index.bzl", "ts_library")

prerender_component(
    name = "my_component",
    prerender = ":my_component_prerender",
    client_scripts = ":my_component_scripts",
    styles = ":my_component_styles".
    resources = ":my_component_resources",
)

ts_library(
    name = "my_component_prerender",
    srcs = ["my_component_prerender.ts"],
   # Dependency on the prerender re-export of another `prerender_component()`.
    deps = ["//component_a:component_a_prerender"],
)

ts_library(
    name = "my_component_scripts",
    srcs = ["my_component_scripts.ts"],
   # Dependency on the scripts re-export of another `prerender_component()`.
    deps = ["//component_b:component_b_scripts"],
)

filegroup(
    name = "my_component_styles",
    srcs = [
        "my_component_styles.css",
        # Directly depend on the styles of another component.
        "//component_c:component_c_styles",
    ],
)

prerender_resources(
    name = "my_component_resources",
    entries = {
        "/foo.json": ":foo.json",
    },
    # Directly depend on the resources of another component.
    # This kind of break abstraction, but resources can have semantic dependencies on each so 🤷?
    deps = ["//component_d:component_d_resources"],
)

Each slice of the component explicitly lists its own dependencies on the re-exports of other prerender_component() targets. The prerender_component() itself just collects all the different slices and re-exports them at a consistent location. This makes prerender_component() much simpler and gives the user more control over each piece of their component (they can control the tsconfig directly, they can use js_library(), they can make their own rules, etc).

The challenge here is that prerender_component() has no knowledge of its dependencies, because they are defined one later of indirection away. It can't look at the dependencies of its prerender ts_library() for example. My thought for working around this is to abuse aspect() and invert the dependencies slightly. We can have prerender_component() also generate a private :{name}_metadata target. This would return custom providers that define the files for each slice of the component (prerender JS, client JS, CSS, and resources). That's great, but there's actually no way to depend on this target since prerender_component() doesn't know about it's component dependencies. However, we can cheat the re-export targets. Each of those re-exports can also depend on this :{name}_metadata target and re-export its custom providers as well. This means that all of:

• //component_a:component_a_prerender
• //component_b:component_b_scripts
• //component_c:component_c_styles
• //component_d:component_d_resources

Will be re-exports of the underlying implementation give to the associated prerender_component(), and they will include the extra provider from their associated :{name}_metadata target. This means that ever prerender_component() has a dependency on the metadata for every component depended upon by each slice of the component. Even if you only depend on a component's scripts, you still depend on that metadata target and include its providers. Unfortunately it is still one layer of indirection away across rules I don't own and can't modify, so prerender_component() cannot actually get this provider and look at it.

However, prerender_pages() can through the use of an aspect(). This could walk the dependency graph and look for all the prerender_component() metadata targets it depends upon and collect their custom providers into a single struct that contains all of the prerender JS, client-side JS, CSS, and resources. Then we can mostly follow the design as it already works.

1. Build a nodejs_binary() for the prerender JS.
2. Invoke it with the renderer to generate annotated HTML.
3. Extract annotations (includeScript() and includeStyle() comments) from the HTML.
4. Construct client-side JS and CSS entry points based on the annotations.
5. Bundle the client-side JS and CSS, tree shaking any files not used transitive by the entry point.
6. Inject the relevant tags into the prerendered HTML and dump everything into a directory.

This strategy means that depending upon even just the client-side JS will also bring it the CSS and resources into prerender_pages(). That would be enough for resources to work since we don't and have no motivation to tree shake them. It still doesn't quite solve the CSS problem, since there would be no includeStyle() annotation to inject the CSS for that client-side-only component. I suspect we could find a way to use Constructable StyleSheets to include them in the client-side JS, but that could duplicate the dependency if used in conjunction with a prerendered component. Maybe that's ok and not the end of the world, but I would like to find a better path to supporting prerendered and client-side rendering for the same component. That's more of a tangential issue, but until I have a solution for that there isn't a whole lot of motivation to do the work here since it doesn't give much value without being able to use client-side-only CSS.

The one piece of value this change does give on its own is that it great simplifies prerender_component() and gives the user much more control over their own BUILD.bazel file. So I guess we should keep that in mind as well.

[dgp1130]

Took a quick stab at this today in ref/rewrite-prototype. Actually worked out more smoothly than I thought. No major issues with the approach and I was able to define a component which imported another only via its script, while that script had a required resource which needed to be pulled in. prerender_pages() was able to collect the resource through the aspect and serve it as expected, even though that resource's component's prerender library was never depended upon.

This still has the annoying trade off that users need to depend on the "phantom" :component_prerender targets. I could abstract this a bit with a function or other tricks, but I'm not sure that's necessarily an improvement. Otherwise it works about as well as I hoped. I'll have to keep playing around with it and see if I want to move forward with the design.

[dgp1130]

Trying to think through the negative consequences of this alternate design, and my biggest concern is definitely users having to manipulate separate :component_* targets. Just documenting and explaining this to users is likely to be tricky. However, we could potentially smooth over those rough edges with:

1. A dependency test which fails the build on any bad dependency. I'm not sure how feasible this is, but I have an idea for how it might work, needs some experimentation.
2. A Gazelle extension which automatically manages the imports for you. I have no idea what the current state of the TS/JS ecosystem with Gazelle is. I found https://github.com/benchsci/rules_nodejs_gazelle, but that seems specific to @build_bazel_rules_nodejs. I believe Aspect was hoping to make something happen, but I'm not able to find anything right now. If something exists, we might be able to set some tags to configure it to be compatible with this model? As long as we can tell it to import from :component_prerender instead of :my_prerender_lib, that's probably good enough. Though css_library() obviously won't have any out-of-the-box support, though I'm less concerned about that given that composing CSS libraries like that is a lesser use case.

I attempted a query for 1., but came up short. The best I had was:

let root = //path/to/root:component_metadata in
let metadata = kind(prerender_metadata, deps($root)) in
let component_slices = deps($metadata, 1) except $metadata in
let component_slice_aliases = rdeps(deps($root), $metadata, 1) except $metadata in
let result = rdeps(deps($root), $component_slices, 1) except $metadata except $component_slice_aliases in
$result

This doesn't actually work for two reasons:

1. This relies on looking for a "component slice" with a direct reverse dependency other than it's metadata target. However, identifying a "component slice" relies on finding all the direct dependencies of the prerender_metadata() targets. That sounds fine, except that a component's prerender_metadata() target is only in transitive dependencies if it is depended upon correctly anyways (through a "component slice alias"). So this would only actually catch an incorrect dependency on a component slice if there was already another correct dependency for the same prerender_pages() transitive deps, which is pretty limiting.
2. deps() and rdeps() seem to have a very unfortunate "feature" in that they return the input targets in their output, always. For example:

$ bazel query "deps(//examples/rewrite:site_unbundled_deps_test, 1)"
//examples/rewrite:site_unbundled_component_metadata
//examples/rewrite:site_unbundled_deps_test

In this case, //examples/rewrite:site_unbundled_deps_test is considered it's own dependency, which is just wrong IMHO. This normally isn't that big a deal, since you can just add an except to drop the inputs. However, this actually omits too many targets. If you do something like deps(set(//:foo //:bar)) except set(//:foo //:bar) you would lose any dependency relationship between //:foo and //:bar. For example, if //:foo depends on //:bar, then //:bar is a valid result of deps(set(//:foo //:bar)). However, since //:bar is always a result and must be excluded, the valid //:bar dependency gets incorrectly dropped.

This is a problem for my query because I need to look up bad reverse dependencies on "component slices", but also a "component slice" is the most likely offender with a bad dependency on another "component slice". If we exclude all "component slices" from the output (intending to remove the self-edge of the rdeps() input) we actually allow dependencies from all "component slices" directly on all other "component slices", which is wrong. They should be going through "component slice aliases" instead. I filed https://github.com/bazelbuild/bazel/issues/17777 to see if this behavior is actually intentional, but given that I'm blocked on 1. anyways, I don't know that a dependency test here is doable.

I asked in the Bazel slack about Gazelle support for TypeScript in @aspect_rules_js and apparently it is implemented, just not well documented atm: https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1678841370041579.

• https://docs.aspect.build/v/cli/commands/aspect_configure/
• https://github.com/aspect-build/aspect-cli/tree/890e071/gazelle/js
• https://github.com/aspect-build/aspect-cli/blob/a97db9e/docs/help/topics/directives.md

Maybe the js_resolve directive could get Gazelle to do the right thing? Not totally sure how that's supposed to work. I think these directives are supposed to be in build files though, not managed through tags which could be done in macros. That's unfortunate, though code scaffolding could generate these directives correctly up front. Gazelle does apply directives to all nested packages, so at least they don't need to be duplicated for each package. The directives would likely need to make some assumptions about naming conventions, which is a bit awkward. I have wanted to push more consistent naming conventions like component.prerender.mts, component.client.mts, and component.css. If so, we might be able to do something like # gazelle:js_resolve *.prerender.mts :{dirname}_prerender? Not sure if that totally makes sense.

[dgp1130]

I had a conversation with @jbedard (thanks so much!) about this particular design and how we can make it compatible with Gazelle.

I installed Aspect CLI, enabled the Gazelle JS plugin, and tested it out with aspect configure. Sure enough, it doesn't work out of the box for these patterns. The best configuration I could come up with was:

# Default `ts_project()` target name is `:prerender`.
# gazelle:js_project_naming_convention prerender

# Resolve prerender files to the `:my_component_prerender` target.
# gazelle:js_resolve **/*.prerender.{js,mjs} //{package}:{dirname}_prerender

# Resolve client files to the `:my_component_scripts` target.
# gazelle:js_resolve **/*.client.mjs //{package}:{dirname}_scripts

Unfortunately this doesn't work for two reasons:

1. The Gazelle JS extension only generates one ts_project() and puts all the source files into there (technically two ts_project() targets if you include tests, but that doesn't apply here). This is too broad for @rules_prerender and basically a non-starter. A primary goal of this project is to separate prerendering and client-side dependency structures.
2. The Gazelle JS extension does not expand //{package}:{dirname}_prerender. {dirname} is used in other directives, but not supported for js_resolve. Apparently the goal of js_resolve is to add glob support to gazelle:resolve, so there really isn't any precedent for interpolations like this.

The first problem is a little more straightforward and @jbedard was already looking into it for separate reasons. It should be possible to make a directive which maps a file glob to a target name and then generate and manage multiple targets. I filed https://github.com/aspect-build/aspect-cli/issues/427 to describe my use case and help prioritize the effort.

The second problem is more complicated. Even my example is awkward given that {package} and {dirname} refer to the Bazel package and most nested directory name for the imported file, not the importing file which could be confusing. @jbedard also pointed out that behavior can get even more confusing because Bazel packages are not 1:1 with file system directories, so the exact behavior of importing a //foo:bar/baz.js file is hard to define.

Ultimately the problem here is that I want to do a "dynamic resolve" where I'm trying to describe a generic rule which can apply to any file and map it to a target inferred from the file name. gazelle:js_resolve as it is today is more for "static resolutions" such as # gazelle:js_resolve my-lib //packages/my_lib. It's unclear if gazelle:js_resolve would even be the right mechanism for dynamic resolution, however there does not appear to be a better option right now.

Alternative approach

@jbedard suggested an alternative implementation which might make some better trade offs. Instead of trying to configure Gazelle to detect a dependency on a component's ts_project() and reroute it a generated target from prerender_component(), we could let Gazelle generate a dependency directly on the ts_project() target, but then use macros to swap target names and actually depend on the prerender_component() alias. A simplified implementation would look like:

load("@aspect_rules_ts//ts:defs.bzl", _ts_project = "ts_project")

# Wrap `ts_project()`.
def ts_project(name, **kwargs):
    # Don't use `name`, instead generate this target with a slightly different name.
    # We're assuming that a sibling `prerender_component()` execution will generate `name` and point at `actual_ts_proj`.
    actual_ts_proj = "_%s_lib" % name
    _ts_project(
        name = actual_ts_proj,
        **kwargs
    )

def prerender_component(name, prerender):
    prerender_name = prerender[1:] # Drop leading `:` from input.
    _alias_with_metadata(
        # Create the alias with the same name as the `prerender` input, this is the name the `ts_project()` did *not* use.
        name = prerender_name,
        actual = ":_%s_lib" % prerender_name, # `ts_project()` wrapper should have generated this target.
    )

# my_component/BUILD

# Import `ts_project()` from `@rules_prerender`, *not* `@aspect_rules_ts`.
load("@rules_prerender//:index.bzl", "prerender_component", "ts_project")

# Counter-intuitively generates `:my_component_prerender` as an alias to `:_my_component_prerender_lib`.
prerender_component(
    name = "my_component",
    prerender = ":my_component_prerender",
    # ...
)

# Generates `:_my_component_prerender_lib`, but *not* `:my_component_prerender`.
ts_project(
    name = "my_component_prerender",
    srcs = ["my_component.prerender.ts"],
    # ...
)

# my_other_component/BUILD

# Import `ts_project()` from `@rules_prerender`, *not* `@aspect_rules_ts`.
load("@rules_prerender//:index.bzl", "prerender_component", "ts_project")

prerender_component(
    name = "my_other_component",
    prerender = ":my_other_component_prerender",
    # ...
)

ts_project(
    name = "my_other_component_prerender",
    srcs = ["my_other_component.prerender.ts"],
    # ...
    # Seemingly valid dependency! Gazelle should resolve this correctly without any configuration.
    deps = ["//my_component:my_component_prerender"],
)

This "target swapping" trick is done entirely via macros and is invisible to syntactic BUILD tools like Gazelle. As a result, it only sees a file my_other_component.prerender.ts importing another file my_component.prerender.ts and therefore creates a dependency from //my_other_component:my_other_component_prerender to //my_component:my_component_prerender. This would normally be a bad dependency edge which drops component metadata, but since ts_project() and prerender_component() "swapped" target names, the component alias is actually at //my_component:my_component_prerender and will include component metadata so this is a valid dependency!

This is essentially updating the BUILD file structure to align with Gazelle's default semantics and abusing macros to restructure dependencies under the hood and still get the desired dependency structure. It even has the additional benefit that users not on Gazelle don't have to understand the magic :my_component_{slice} targets, just looking at the build files it is clear what //my_other_component:my_other_component_prerender is depending on! This approach does appear to work with Gazelle, however it comes with some non-trivial trade-offs.

First, this approach requires a @rules_prerender wrapper of ts_project(). It only adjusts the name, so the implementation is relatively straightforward. It could also assert the input name matches *_prerender and *_scripts or limit the rename to only those targets. Whether or not the rule should be named ts_project() or something else is still unclear. ts_prerender_project() would be the obvious alternative, but using ts_prerender_project(name = "my_component_scripts") just feels wrong because the term "prerender" is overloaded as "the name of this project per @rules_prerender" and "the name of the prerender component slice". In this case we would mean the former but in a component BUILD file, users could easily interpret this with the latter meaning and get very confused from building the client-side slice of a component with a "prerender" rule.

It is very easy to accidentally use the @aspect_rules_ts version of ts_project(). I believe we could use # gazelle:map_kind ts_project ts_project @rules_prerender//:index.bzl to potentially make this change for users. However this is not ideal either because:

1. It doesn't help non-Gazelle users.
2. "Existing rules of the old kind will be ignored." - If users accidentally import @aspect_rules_ts, it won't overwrite that with the @rules_prerender version.
3. ts_project() targets used outside prerender_component() should use the @aspect_rules_ts version, however there is no clear way to express that to Gazelle. We could have workspaces define a //my/site/components/... package and put the directive only in there to avoid impacting the whole workspace, however this is not very precise.
4. When users do use the wrong ts_project() they would likely get an error message like ":my_component_prerender is already declared", which isn't very easy to link to "you imported the wrong ts_project()".

Part of my motivation for this design as well is to move off the ts_project() dependency a little more cleanly. Re-exporting my own wrapper of ts_project() makes it harder to avoid shipping a hard dependency on @aspect_rules_ts. I do think it's possible by just moving the ts_project() wrapper to a separate Starlark file in a separate package like so:

# Most imports come from here.
load("@rules_prerender//:index.bzl", "...")

# `ts_project()` wrapper comes from `//ts:defs.bzl`.
load("@rules_prerender//ts:defs.bzl", "ts_project")

This way the user should never execute load("@aspect_rules_ts", "...") unless they directly load ts_project(), which would only be when they actually need the TS dependency. I think that should make this ergonomic enough without requiring an @aspect_rules_ts dependency.

Second, this approach effectively builds the recommended naming conventions into the macros. Restricting or modifying behavior based on the _prerender or _client name forces users into this naming convention and would lead to confusing behavior if not followed. With Gazelle I was hoping to push users towards this convention by default, but not strictly require it.

Third, prerender_component() and ts_project() are now awkwardly coupled together in a really unintuitive way. This leads to a few unintended effects:

1. ts_project() does not generate a target with the name it is given. This is super counter-intuitive, since even a simple bazel build //path/to/pkg:my_ts_project no longer works as expected. A sibling prerender_component() should set up this alias correctly, but if it is forgotten by the user this will lead to very confusing error messages.
2. prerender_component() and its direct dependency ts_project() targets must be in the same Bazel package, otherwise prerender_component() can't steal its name.
3. There is no obvious way to detect if a ts_project() is declared without a sibling prerender_component() to generate its alias. Even hacky side-effectful macro tricks couldn't really make this work without requiring that prerender_component() comes first in the BUILD file, which is a very weird restriction.

Fourth, we would need to apply this "target swapping" trick to css_library() and web_resources() for consistency. Fortunately I own both of these rules (though I sure wish I didn't have to own css_library()) so we don't need to do any awkward wrapping. Still an unintuitive behavior though and runs into the same restrictions as above.

Fifth, visibility gets confusing because the user expects the ts_project(name = "my_component_prerender", visibility = ["..."]) to dictate its visibility, but really the prerender_component() decides visibility. I'd actually argue that the ts_project() should almost always be private, given that we want all incoming dependencies to go through the component alias, but this can be confusing. Since we need the prerender_component() to be in the same package anyways, maybe we should just outright fail() on any visibility and tell users to modify the prerender_component() instead? Not exactly intuitive, but at least we can detect the bad behavior and redirect users towards the solution. compatible_with and restricted_to could get confusing as well, given that you'd get an error message that :my_component_prerender needs compatible_with, but you actually need to specify it on the prerender_component() target, not the ts_project().

Some of these can be addressed by generating a component and its BUILD file in the correct shape up front, however we can't always rely on users going through that codegen, so whatever API we use needs to align with human understanding of BUILD files.

One possible benefit of wrapping the ts_project() rule is that we can potentially solve the dependency test problem. We can make ts_project() export some kind of IAmAComponentSliceInfo provider to declare that the target is a component slice without necessarily having to find a reverse dependency on a :my_component_metadata target. We could then choose not to re-export this provider in the component alias. This gives us a little more information, and I suspect we could create an aspect which looks for a dependency from one component slice target onto another component slice target (as identified by the IAmAComponentSliceInfo provider). If we find such a dependency edge, then we have a direct dependency which is not going through a component alias and is therefore a bad dependency which needs to be adjusted.

Unfortunately I don't think it's that useful with this alternative "target swapping" approach. This would only catch a mistake when it is already using the correct ts_project() implementation, and since we're swapping the target names, it would be really hard to accidentally depend on :_my_component_prerender_lib. This might be more useful in the original approach, but that doesn't require wrapping ts_project(), so the prerender and client slices won't have IAmAComponentSliceInfo and the whole thing falls apart. We could potentially use this in css_library() and web_resources(), since both of those are owned by @rules_prerender. However those are also the least likely to make this particular mistake, given that CSS-only and resource-only dependencies are by far the most rare.

I'll have to think more about this, but my immediate reaction is that this alternative approach has too many rough edges to be better than the original proposal.

[dgp1130]

Another idea came to me last night on validation. We actually can apply the IAmAComponentSliceInfo trick to the original design, we just have to wrap ts_project() to do it. Imagine:

load("@rules_prerender//:index.bzl", "prerender_component", "wrapped_ts_project")

prerender_component(
    name = "my_component",
    prerender = ":prerender",
    scripts = ":scripts",
)

wrapped_ts_project(
    name = "prerender",
    srcs = ["my_component.prerender.mts"],
)

wrapped_ts_project(
    name = "scripts",
    srcs = ["my_component.client.mts"],
)

wrapped_ts_project() would generate a ts_project() and then also add an IAmAComponentSliceInfo provider.

load("@aspect_rules_ts//ts:defs.bzl", "ts_project")

def wrapped_ts_project(name, **kwargs):
    wrapped_name = "_%s_lib" % name
    _alias_with_slice_provider(
        name = name,
        actual = ":%s" % wrapped_name,
    )

    ts_project(
        name = wrapped_name,
        **kwargs
    )

def _alias_with_slice_provider_impl(ctx):
    return [
        # Re-export everything notable.
        ctx.attr.actual[DefaultInfo],
        ctx.attr.actual[JsInfo],

        # Also provide `IAmAComponentSliceInfo` to denote this target as a component slice.
        IAmAComponentSliceInfo(),
    ]

_alias_with_slice_provider = rule(
    implementation = _alias_with_slice_provider_impl,
    attrs = {
        "actual": attr.label(
            mandatory = True,
            providers = [JsInfo],
        ),
    },
)

Users can treat wrapped_ts_project() just like a normal ts_project(). Then prerender_component() introduces a restriction that all it's direct dependencies must provide IAmAComponentSliceInfo. This makes sure a user always annotates component slice targets. Finally, prerender_component() can apply an aspect that processes the transitive deps tree and throws an error for any dependency on a component slice which is not coming from a prerender_component() target (specifically the metadata target). So I think we can write something like:

if PrerenderMetadataInfo not in ctx.target: # The aspect is processing a target which is _not_ a `prerender_component()`.
    for dep in ctx.attr.deps:
        if IAmAComponentSliceInfo in dep: # Found a dependency which is a slice of a `prerender_component()`.
            fail("Bad dependency on a component slice!")

The aspect would essentially detect and fail for any dependencies on a component slice which did not go through the prerender_component() target, which is exactly what we want. This doesn't at all help with Gazelle or anything like that, but it does put some strong guardrails in place to keep users on the right path. We can even make this a build error rather than a test failure to catch the mistake sooner.

The main trade-off here is that we need to wrap or intercept the ts_project() dependency so we can effectively mark the target as a component slice, that way any other dependencies are able to check (through the aspect) that they aren't inadvertently depending on a component slice which should have gone through a prerender_component().

I would love to make this wrapping implicit in the prerender_component() macro, but that would not work because any incorrect dependencies on a component slice need to depend on the wrapped target with the IAmAComponentSliceInfo provider, so the wrapping has to happen outside the prerender_component() target. To further add to this, we need to do the same thing for css_library() and web_resources() but can't do it automatically. The whole point of IAmAComponentSliceInfo is that only targets used as component slices have the provider. If we added it by default to all ts_project(), css_library(), or web_resources() targets then users wouldn't be able to depend on anything.

Given that CSS-only and resource-only dependencies between components are pretty rare, we could potentially just ignore that case and focus on ts_project() which covers prerendering and client-side scripts. It would also work for SSR if we ever got around to that. However the goal of this issue is to keep related resources for client-side scripts and styles. A CSS-only dependency which includes a font would easily be missed if we didn't apply this same pattern. Of course, this is only a validation check. You can absolutely still have a CSS library which depends on a font file, we just won't throw a useful error if you depend on the css_library() directly, rather than its associated prerender_library().

The fundamental problem is that the whole idea relies on the fact that a give target knows that it will only be used as a component slice, which is fundamentally inverting the dependency graph. A target should not know anything about how it's used, but here we're making a pretty strong assumption about the target's usage.

Another-nother idea is that if you use visibility correctly and keep each component in its own package, that should provide some reasonable validation as well, though with a notably worse error message. We could lean into that harder and make an aspect which returns the visibility of any given target. Then prerender_component() could check it's direct dependencies to make sure they have private visibility? This would assume that there is only one prerender_component() per package or at least that packages are small enough that most dependencies between components are cross-package. I think the main takeaway from this issue is that aspect are super ~abusable~ useful.

To refocus on the core problem at hand though, I still think the proposed design in the OP of this issue is objectively better than what we currently have and viable to implement per my prototype. It does solve the primary issue of expressing dependencies between non-prerender slices. The biggest problem is just about confusion between :my_component_prerender and :prerender, and there are four possible approaches to mitigate this problem:

1. Add proper Gazelle support so it does this for users automatically.
   • This doesn't seem possible today, but could become possible in the future. I don't think the design is inherently incompatible with Gazelle.
   • Even if we had better Gazelle integration, some users won't be using Gazelle and run into the same problems.
2. Make :my_component_prerender obviously the correct dependency by making the prerender_component() and ts_project() macros "swap" their targets.
   • The BUILD files look much more intuitive with this approach, you depend on the target you think you should depend on.
   • Works well with Gazelle.
   • Has a lot of unintuitive side effects and assumptions baked into it. Lots of rough edges if you get minor things wrong with little to no ability to detect those mistakes and give good error messages.
3. Provide a good error when users accidentally depend on the wrong target.
   • Can probably do this via an aspect, and even fail at build time.
   • Should be able to present very good error messages.
   • Requires wrapping ts_project(), css_library(), and web_resources() in a really awkward way.
   • Don't have to wrap css_library() and web_resources() if we don't want to, but they won't get this validation.
4. Validate component slices have private visibility and rely on that to prevent bad dependencies.
   • Very unobtrusive to implement, doesn't modify the BUILD file at all.
   • Component slices would be private by default, so this isn't that much different over doing nothing. It doesn't so much catch bad dependencies as catch component slices with overly broad visibility.
   • Doesn't present a great error message, since incorrect dependencies would just get a visibility error. But if the user tries to add visibility, they would get a much more informative error message.

I'm still not quite decided here, but I think it's worth experimenting with 3. and 4. to validate that they are indeed possible and see how well they work out in practice. I feel like 4. in particular is unobtrusive enough that there's no reason not to do it? 🤷 I think the big question is if 1. - 3. make any better trade-offs (and right now I'm not convinced that they do), or if there's a magical 5th option which 10 out of 10 dentists recommend. 🤔💭

[dgp1130]

The question of what to do here has been stewing in my mind for a while. Looking back on the 4 options:

1. Gazelle integration isn't possible today, but I don't see this as a reason not to move forward with the redesign.
2. "Swapping" component slices is way too magical and leaks in too many ways. I don't think I want to do this at all.
3. I'm not convinced wrapping component slice targets is worth the gain. This is confusing and breaks abstraction pretty heavily.
4. I prototyped a visibility check and was able to make it work. This does restrict the project structure of @rules_prerender somewhat. Notably a prerender_component and its slices must be in the same Bazel package. Also all prerender component slices must be visibility private. I tried to give some good error messages, but I can see some users getting tripped up by it. This mostly aligns with what I would consider "best practices" anyways, so I think it's worth trying. We can always remove or relax the check later if necessary.

I'm inclined to move forward using 4. as the only protection mechanism. It won't catch bad dependencies between components in the same package. I expect most components will get their own package, and packages with multiple prerender_component targets shouldn't have a massive number of them. If you have O(100) components in the same package, I think any dependency problems are kinda on you.

Depending on how well 4. works in practice, we can reevaluate on 1. - 3.

[dgp1130]

Landed all the relevant changes and included some pretty extensive documentation. Would love to do more of that and get some of this content out of my head.

In 0.0.30 I launched the new changes under prerender_component2 without breaking prerender_component. If there's anyone out there actually using @rules_prerender (why u no tell me?), then you should migrate everything to prerender_component2 in that version.

In 0.0.31, the existing prerender_component is deleted and prerender_component2 is renamed to prerender_component. This will be the only implementation moving forward.

I was able to migrate all my example apps without too much effort. I still think this is a better approach than what we had before. Time will tell if the visibility check is sufficient to catch bad usages.