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

[FR]: types_only() #319

Open
1 task
dgp1130 opened this issue Feb 14, 2023 · 4 comments
Open
1 task

[FR]: types_only() #319

dgp1130 opened this issue Feb 14, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@dgp1130
Copy link
Contributor

dgp1130 commented Feb 14, 2023

What is the current behavior?

ts_project() generally provides a JsInfo with .d.ts declarations and .js sources, which is typically what you want in most situations. However, sometimes you want to type check against a dependency without including the implementation of that dependency. This can happen because you expect the implementation to be provided as an input or linked separately. In my particular situation, I would like to type check a tool against my own npm_package() built at HEAD but not include it in the output tool's implementation, since it will be dynamically linked against the same npm_package() in the user's workspace. See dgp1130/rules_prerender#48 (comment) for more context.

You can technically do this today with emit_declaration_only however this has two problems when applied to this use case:

  1. It only applies to ts_project(), not npm_package().
  2. It must be applied to the ts_project() dependency, not the usage of that dependency. When package foo is built, it should contain its full implementation. However a particular dependent of it may want to limit itself to just consuming the types of foo, rather than the full *.js.

You can also do this with import type which makes TypeScript prevent value references to that import. However there is nothing stopping misguided developers from adding a value import in the future. Also import type is not known to the Bazel layer and does not prevent emitting the JavaScript. This will still pull JavaScript from the type-only dependency, even if the import is elided at runtime. This can complicate dependency resolution, particularly in cases of multiple workspaces. You'd need to pass through a bundler to fully tree shake the unused JS implementation, and that shouldn't be needed for something like this.

Describe the feature

I propose a types_only() rule which effectively strips the JS implementation from the JsInfo provider. You would use it like so:

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

ts_project(
    name = "lib",
    srcs = ["lib.d.ts"],
    declaration = True,
    # ...
)

# Use normally with JS sources included.
ts_project(
    name = "app",
    deps = [":lib"],
)

types_only(
    name = "lib_types",
    dep = ":lib",
)

# Depend only on the types, `lib.js` is not in the transitive output.
# `tsc` will error on value usages of the dependency.
ts_project(
    name = "app_with_lib_types",
    deps = [":lib_types"],
)

It implementation is pretty straightforward:

load("@aspect_rules_js//js:providers.bzl", "JsInfo", "js_info")

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.",
)

Since npm_link_package() returns a JsInfo as well, this same implementation works there. However it has the caveat that NPM packages are implemented with a TreeArtifact in the declarations / transitive_declarations property. That TreeArtifact still maintains *.js sources, even if all the other properties of the provider are dropped. That means that if you use types_only() on an npm_link_package(), tsc will actually pass though the output won't include the *.js files. Ideally the *.js files would be removed from the NPM package and the package.json would be limited to just .d.ts, though I don't see an easily feasible way of doing that. Not sure what the best approach is there beyond accepting the foot gun that tsc will allow value references for dependents of types_only() of NPM packages.

Fund our work

@dgp1130 dgp1130 added the enhancement New feature or request label Feb 14, 2023
@github-actions github-actions bot added the untriaged Requires traige label Feb 14, 2023
dgp1130 added a commit to dgp1130/rules_prerender that referenced this issue Feb 14, 2023
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.
@gregmagolan gregmagolan moved this to 📋 Backlog in Open Source Feb 14, 2023
@gregmagolan gregmagolan removed the untriaged Requires traige label Feb 14, 2023
@neal-codaio
Copy link

We have a moderately large project with thousands of rules and tests. We auto generate all our dependencies and we use consistent-type-imports. The combination of all these things makes types_only attractive to us as an optimization. We use this within our build on every PR.

However, to ensure that we aren't over caching, we needed to make one change to the implementation provided above.

-        sources = depset(),
+        sources = info.declarations,

With that change, we get the appropriate caching behavior. A test (eg) will wait for a dependent ts_project (types_only) rule to build, but then if it only depends on types_only, the test result will be cached since it didn't need the implementation. This change looks like it saved several minutes on our build and test runs at the 95%-ile.

@gregmagolan
Copy link
Member

gregmagolan commented Jan 23, 2024

This feature can also be useful for the cases where you have test targets that take .ts or .tsx files as inputs and the test runner (such as jest or mocha) do the transpiling and there are import type in the typescript files. In those cases, not depending on the runtime of those deps could result in better caching.

It is important to note, however, that under Bazel the better pattern is to pre-transpile all .ts and .tsx files with ts_project targets so test runners only deal with the transpiled .js files.

@dgp1130
Copy link
Contributor Author

dgp1130 commented Feb 15, 2024

Just discovered that Angular has an implementation of this called extract_types in our current @build_bazel_rules_nodejs workspace. Just one more example use case for motivation here.

https://github.com/angular/dev-infra/blob/8642bb38e4e37508133aee57c8be9c3903495289/bazel/extract_types.bzl

@dgp1130
Copy link
Contributor Author

dgp1130 commented Jul 29, 2024

The proposed implementation here is no longer viable as of @aspect_rules_js v1.40.1. aspect-build/rules_js#1554 removes the declarations properties this depended upon.

Instead I went with a different approach of using a regular dependency, created an NPM package, and then afterwards "pruned" the type-only dependency after the fact. It's not super elegant, but works well enough.

dgp1130/rules_prerender@3893834...313314e

My implementation is limited to pruning from npm_package targets, but in theory it should support pruning ts_project directly. Unfortunately the JsInfo type is quite a bit more complicated than NpmPackageInfo, but in theory might be doable.

An alternative approach is to just import type everything you need, TS import elision will naturally ensure it doesn't get loaded at runtime. However it's hard to guarantee that and you likely need to add extra tests or checks to ensure future PRs don't accidentally introduce a value import when you don't have a types_only dependency to cause that error for you.

It would definitely be great for this to be a supported feature so we don't need these workarounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 📋 Backlog
Development

No branches or pull requests

3 participants