Skip to content

Commit

Permalink
Adds types_only() to limit renderer's dependency on rules_prerender.
Browse files Browse the repository at this point in the history
Refs #48.

The renderer should not depend on `rules_prerender` directly, because that would pull in the JavaScript sources into `.../execroot/rules_prerender/node_modules/rules_prerender/` of the generated binary. The `rules_prerender` dependency should actually be linked against the user's implementation in their own workspace (`//:node_modules/rules_prerender`), not `@rules_prerender//:node_modules/rules_prerender`. However, we still want type checking of `rules_prerender` usage, even if the actual package gets injected via the main function parameter.

`types_only()` accomplishes this by dropping all the JS sources and only propagating TypeScript declarations. This prevents the `rules_prerender` package from `@rules_prerender//:node_modules/rules_prerender` leaking into the renderer binary, so there is only one definition of it which comes from the user's workspace. `import type` also helps restrict that the dependency should _only_ ever be used as type.

Unfortunately, `npm_link_package()` emits a `declarations` field which points to the NPM package's `TreeArtifact`, and that tree contains `*.js` files. That means TypeScript actually _does_ receive `*.js` files and is generally ok with value imports of `rules_prerender`. There's also no easy way of removing `*.js` files from that tree. This is an unfortunate foot-gun, but the most important thing is that the `*.js` files don't persist at runtime, even if the compiler may allow it.

I filed a feature request to `@aspect_rules_ts` with this general idea and also explains more the `npm_link_package()` foot-gun: aspect-build/rules_ts#319.
  • Loading branch information
dgp1130 committed Feb 14, 2023
1 parent 8183ffd commit 15e278c
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 22 deletions.
6 changes: 6 additions & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ load("@npm_rules_js//:defs.bzl", "npm_link_all_packages")
load("//tools:publish.bzl", "publish_files")
load("//tools/publish:npm_publish.bzl", "npm_publish")
load("//tools/stamping:stamp_package.bzl", "stamp_package")
load("//tools:typescript.bzl", "types_only")
load("//:index.bzl", "link_prerender_component", "prerender_component", "prerender_component_publish_files")

exports_files([".npmrc"], visibility = ["//visibility:public"])
Expand Down Expand Up @@ -54,6 +55,11 @@ npm_link_package(
src = ":rules_prerender_pkg",
visibility = ["//visibility:public"],
)
types_only(
name = "node_modules_types/rules_prerender",
dep = "//:node_modules/rules_prerender",
visibility = ["//visibility:public"],
)

npm_link_package(
name = "node_modules/@rules_prerender/declarative_shadow_dom",
Expand Down
1 change: 1 addition & 0 deletions packages/renderer/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ ts_project(
"//common:formatters",
"//:node_modules/@types/node",
"//:node_modules/@types/yargs",
"//:node_modules_types/rules_prerender",
],
)

Expand Down
28 changes: 6 additions & 22 deletions packages/renderer/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,11 @@ import type { MainFn } from '../../common/binary';
import { mdSpacing } from '../../common/formatters';
import { invoke } from './entry_point';

// Can't import `rules_prerender` so we duplicate enough of the definitions to
// do what the renderer needs. We _should_ be able to just `import type` and
// simply make sure we don't use a value reference of the import, but this
// doesn't work with `@aspect_rules_ts` which seems to not use the dependency at
// all. See: https://github.com/dgp1130/rules_prerender/issues/48#issuecomment-1420299784
// TODO(#48): Import just this type instead of duplicating the definitions.
interface RulesPrerender {
readonly PrerenderResource: typeof PrerenderResource,
internalSetInlineStyleMap(map: Map<string, string>): void;
readonly InternalInlineStyleNotFoundError: typeof InlineStyleNotFoundError,
}

declare class PrerenderResource {
readonly path: string;
readonly contents: ArrayBuffer;
}

declare class InlineStyleNotFoundError extends Error {
readonly importPath: string;
readonly availableImportPaths: string[];
}
// Cannot include a value reference of `rules_prerender` because the user will
// depend on it via the `npm_link_package()` in their own workspace, not the
// `@rules_prerender//:node_modules/rules_prerender` dependency we use for type
// checking.
import type * as RulesPrerender from 'rules_prerender';

/**
* Creates the renderer's main function for the binary. We accept
Expand All @@ -34,7 +18,7 @@ declare class InlineStyleNotFoundError extends Error {
* not the `@rules_prerender` workspace.
*/
export function createRenderer(
rulesPrerender: RulesPrerender,
rulesPrerender: typeof RulesPrerender,
entryModule: unknown,
entryPoint: string,
): MainFn {
Expand Down
27 changes: 27 additions & 0 deletions tools/typescript.bzl
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
load("@aspect_rules_js//js:providers.bzl", "JsInfo", "js_info")
load("@aspect_rules_ts//ts:defs.bzl", _ts_project = "ts_project")

def ts_project(name, tsconfig = None, **kwargs):
Expand All @@ -8,3 +9,29 @@ def ts_project(name, tsconfig = None, **kwargs):
source_map = True,
**kwargs
)

def _types_only_impl(ctx):
info = ctx.attr.dep[JsInfo]
return js_info(
declarations = info.declarations,
npm_linked_package_files = depset(),
npm_linked_packages = depset(),
npm_package_store_deps = depset(),
sources = depset(),
transitive_declarations = info.transitive_declarations,
transitive_npm_linked_package_files = depset(),
transitive_npm_linked_packages = depset(),
transitive_sources = depset(),
)

types_only = rule(
implementation = _types_only_impl,
attrs = {
"dep": attr.label(
mandatory = True,
providers = [JsInfo],
doc = "`ts_project()` to remove JS implementation from."
),
},
doc = "Provides only the types of the given dependency, no implementation.",
)

0 comments on commit 15e278c

Please sign in to comment.