Client script and style-only dependencies drop related resources #40
Comments
|
To elaborate a little more, The problem here is that
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 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 The challenge here is that
Will be re-exports of the underlying implementation give to the associated However,
This strategy means that depending upon even just the client-side JS will also bring it the CSS and resources into The one piece of value this change does give on its own is that it great simplifies |
|
Took a quick stab at this today in This still has the annoying trade off that users need to depend on the "phantom" |
|
Trying to think through the negative consequences of this alternate design, and my biggest concern is definitely users having to manipulate separate
I attempted a query for 1., but came up short. The best I had was: This doesn't actually work for two reasons:
In this case, 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 I asked in the Bazel slack about Gazelle support for TypeScript in
Maybe the |
|
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 # 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}_scriptsUnfortunately this doesn't work for two reasons:
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 aspect-build/aspect-cli#427 to describe my use case and help prioritize the effort. The second problem is more complicated. Even my example is awkward given that 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. 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 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 This is essentially updating the First, this approach requires a It is very easy to accidentally use the
Part of my motivation for this design as well is to move off the # 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 Second, this approach effectively builds the recommended naming conventions into the macros. Restricting or modifying behavior based on the Third,
Fourth, we would need to apply this "target swapping" trick to Fifth, visibility gets confusing because the user expects the Some of these can be addressed by generating a component and its One possible benefit of wrapping the 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 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. |
|
Another idea came to me last night on validation. We actually can apply the 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"],
)
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 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 The main trade-off here is that we need to wrap or intercept the I would love to make this wrapping implicit in the Given that CSS-only and resource-only dependencies between components are pretty rare, we could potentially just ignore that case and focus on 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 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
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. 🤔💭 |
|
The question of what to do here has been stewing in my mind for a while. Looking back on the 4 options:
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 Depending on how well 4. works in practice, we can reevaluate on 1. - 3. |
…rMetadataInfo`. See #40 (comment) for full design explanation.
…rMetadataInfo`. See #40 (comment) for full design explanation. This doesn't actually change the existing `prerender_component`. Instead it creates a new `prerender_component2` with the new implementation. The same is done for `prerender_pages_unbundled2` and `prerender_pages2`. Once all existing examples have been migrated to the new implementation, the old one will be deleted, and these macros will be renamed.
…rMetadataInfo`. See #40 (comment) for full design explanation. This doesn't actually change the existing `prerender_component`. Instead it creates a new `prerender_component2` with the new implementation. The same is done for `prerender_pages_unbundled2` and `prerender_pages2`. Once all existing examples have been migrated to the new implementation, the old one will be deleted, and these macros will be renamed.
This uses an aspect so `prerender_component` can inspect its component slices dependencies. The aspect provides the `visibility` attribute which `prerender_component` asserts on. The idea is that slices for a component _must_ be defined in the same package as the component _and_ must have private visibility. With both of these and assuming a structure where each `prerender_component` gets its own Bazel package, it should be impossible to accidentally depend on a component slice without going through the `prerender_component` reexports. This implements option 4 in [this dicussion](#40 (comment)).
…rMetadataInfo`. See #40 (comment) for full design explanation. This doesn't actually change the existing `prerender_component`. Instead it creates a new `prerender_component2` with the new implementation. The same is done for `prerender_pages_unbundled2` and `prerender_pages2`. Once all existing examples have been migrated to the new implementation, the old one will be deleted, and these macros will be renamed.
This uses an aspect so `prerender_component` can inspect its component slices dependencies. The aspect provides the `visibility` attribute which `prerender_component` asserts on. The idea is that slices for a component _must_ be defined in the same package as the component _and_ must have private visibility. With both of these and assuming a structure where each `prerender_component` gets its own Bazel package, it should be impossible to accidentally depend on a component slice without going through the `prerender_component` reexports. This implements option 4 in [this dicussion](#40 (comment)).
…rMetadataInfo`. See #40 (comment) for full design explanation. This doesn't actually change the existing `prerender_component`. Instead it creates a new `prerender_component2` with the new implementation. The same is done for `prerender_pages_unbundled2` and `prerender_pages2`. Once all existing examples have been migrated to the new implementation, the old one will be deleted, and these macros will be renamed.
This uses an aspect so `prerender_component` can inspect its component slices dependencies. The aspect provides the `visibility` attribute which `prerender_component` asserts on. The idea is that slices for a component _must_ be defined in the same package as the component _and_ must have private visibility. With both of these and assuming a structure where each `prerender_component` gets its own Bazel package, it should be impossible to accidentally depend on a component slice without going through the `prerender_component` reexports. This implements option 4 in [this dicussion](#40 (comment)).
…rMetadataInfo`. See #40 (comment) for full design explanation. This doesn't actually change the existing `prerender_component`. Instead it creates a new `prerender_component2` with the new implementation. The same is done for `prerender_pages_unbundled2` and `prerender_pages2`. Once all existing examples have been migrated to the new implementation, the old one will be deleted, and these macros will be renamed.
This uses an aspect so `prerender_component` can inspect its component slices dependencies. The aspect provides the `visibility` attribute which `prerender_component` asserts on. The idea is that slices for a component _must_ be defined in the same package as the component _and_ must have private visibility. With both of these and assuming a structure where each `prerender_component` gets its own Bazel package, it should be impossible to accidentally depend on a component slice without going through the `prerender_component` reexports. This implements option 4 in [this dicussion](#40 (comment)).
Refs #40. This makes three changes: 1. Adds `CssInfo` to a couple required places. 2. Makes `merge_import_maps` public. 3. Updates `link_prerender_component` to use `css_group` to provide `CssInfo` and `CssImportMapInfo`. All of these changes will be needed in the upcoming change to `prerender_component` which requires CSS providers to be more consistently passed around.
…rMetadataInfo`. Refs #40. See #40 (comment) for full design explanation. This doesn't actually change the existing `prerender_component`. Instead it creates a new `prerender_component2` with the new implementation. The same is done for `prerender_pages_unbundled2` and `prerender_pages2`. Once all existing examples have been migrated to the new implementation, the old one will be deleted, and these macros will be renamed.
…omponent` rewrite. Refs #40. This generates a component metadata along with aliases linked to it. Works with both the old and new `prerender_component` implementations.
Refs #40. This uses an aspect so `prerender_component` can inspect its component slices dependencies. The aspect provides the `visibility` attribute which `prerender_component` asserts on. The idea is that slices for a component _must_ be defined in the same package as the component _and_ must have private visibility. With both of these and assuming a structure where each `prerender_component` gets its own Bazel package, it should be impossible to accidentally depend on a component slice without going through the `prerender_component` reexports. This implements option 4 in [this dicussion](#40 (comment)).
Refs #40. This uses `prerender_component2` and friends. It is a temporary example for testing the rewrite and will probably be deleted once the rest of the repository is migrated to the new implementation.
Refs #40. This dependency edge is only through a client-side script, not the prerender code. A client side script is pulled in from a dependency component which reads a text file. This text file is only included in the output if `@rules_prerender` is able to find it correctly. This verifies the new functionality of `prerender_component2` and confirms that script and style-only deps are supported.
Refs #40. Dropped a now-unnecessary JS dependency, since we're deferring to a `js_library` one step earlier in the build abstraction.
…erender_component2`. Refs #40. Most notably, `prerender_component2` does not create targets for `_styles` and `_resources` if they aren't needed, so we need to optionally skip processing them.
…use the `prerender_component2` implementations. Refs #40.
Refs #40. These have now been superseeded by `prerender_component2` and its associates. I commented out the `bzl_library` targets for now as they aren't really validated but will be needed once they new rules are renamed.
Refs #40. Also does the same for `prerender_pages_unbundled2` and `prerender_pages2`. Re-enables the `bzl_library` targets and updates dependencies/visibility as necessary to match changes in these macros.
…omponent`. Refs #40. The implementation is the same, this is a no-op change.
Refs #40. These are now named `prerender_component` (without the 2) and should be used as such.
Refs #40. For now this gives a rough timeline of how builds work "Life of a build" and a deep dive into `prerender_component` and the how/why of its constraints. In the future, we should document how publishing/linking components works, since that's a particularly tricky area of complexity.
|
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 In 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. |
Refs #40. This is no longer needed since everything is using the new implementation.
On mobile, so I'll need to elaborate more later. But I'm pretty sure that when a
prerender_component()depends on anotherprerender_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 ofprerender_component()whose broad scope has been bothering me for a long time now.The text was updated successfully, but these errors were encountered: