[Bug]: JSON files isn't available to downstream ts_library type checking

pyrocat101 commented 9 months ago

What happened?

When resolveJsonModule is set, TypeScript type check will read JSON files to infer the types. For example:

# foo/BUILD.bazel
ts_library(
    name = "foo",
    srcs = ["foo.ts", "data.json"],
    data = ["data.json"]
    declaration = True,
    tsconfig = "//:tsconfig",  # resolveJsonModule is true in this file
    visibility = ["//visibility:public"],
)

# bar/BUILD.bazel
ts_library(
    name = "bar",
    deps = ["//bar"]
    srcs = ["bar.ts"],
    declaration = True,
    tsconfig = "//:tsconfig",
)

// foo/foo.js
import data from './data.json';
export type Data = typeof Data;

// bar/bar.js
import type {Data} from '../foo/foo';

Type checking //bar will throw an error that data.json cannot be resolved. The file is not present in the runfiles because only .d.ts files of foo are present.

Version

Development (host) and target OS/architectures:

Output of bazel --version: aspect 5.8.19

Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file: rules_ts 2.1.0, rules_js 1.34.0

Language(s) and/or frameworks involved: TypeScript

How to reproduce

Minimal repro: https://github.com/pyrocat101/ts-json-repro

bazel build //bar

Any other information?

No response

alexeagle commented 9 months ago

Yeah I think technically this is a typescript tsc issue, in that it doesn't copy the .json inputs to the --outDir last time I researched it. I think we could copy the file ourselves as a separate action, but it feels like a workaround, so someone should understand at least whether this is a bug in typescript first. Do you have some time to contribute?

alexeagle commented 9 months ago

Ah, first of all it took me a while to repro within rules_ts because we set --skipLibCheck on all our tsc invocations.

I see https://github.com/aspect-build/rules_ts/commit/12012a4bbc32237ae002ba4950b8ef5ffad340d9 actually added the explicit .json outputs, so my previous comment was wrong.

It looks like that PR put the .json outputs into the "js" output group, rather than the "types".

alexeagle commented 9 months ago

I have spent a few hours digging into this, and I'm still not sure it's possible. https://github.com/microsoft/TypeScript/issues/24744 is the open issue in TypeScript that reflects the underlying constraint.

TypeScript handles .json files under --resolveJsonModule in a way that's not the same as other typings. Here's the repro for reference: https://github.com/aspect-build/rules_ts/compare/i516

It's easy to repro by cloning the i516 branch and bazel build //examples/resolve_json_module/bar.

@gzm0 does this make sense to you? Any ideas?

Detailed analysis

.json files are output even with declaration=False, so it's correct that we model them as js_outs in #322. It wouldn't be the right answer to just treat them as typings instead.
We can include those .json files in the typings output_group and transitive_declarations as well, see the second commit on that PR. That way they are propagated through the declarations provider to dependent actions where they should be available as inputs to the program.

--- a/ts/private/ts_project.bzl
+++ b/ts/private/ts_project.bzl
@@ -210,7 +210,13 @@ This might be because
 This is an error because Bazel does not run actions unless their outputs are needed for the requested targets to build.
 """)

-    output_declarations = typings_outs + typing_maps_outs + typings_srcs
+    output_declarations = typings_outs + typing_maps_outs + typings_srcs + [
+        # Probably not the right spot: make .json outputs also appear in typings
+        # so they are propagated to dependents and available for typechecking
+        s
+        for s in output_sources
+        if s.path.endswith(".json") and ctx.attr.resolve_json_module
+    ]

     # Default outputs (DefaultInfo files) is what you see on the command-line for a built
     # library, and determines what files are used by a simple non-provider-aware downstream

(note, we have to patch this after the "Add JS inputs that collide with outputs" occurs, since data.json -> data.json couldn't be pre-declared)

Now we move to the ts_project that depends on that one. tsc thinks that any .json files in the program should be emitted, and emitted files must come from sources that are under the --rootDir. So when .json files appear in the program we get an error

examples/resolve_json_module/index.d.ts(1,18): error TS6059: File '/private/var/tmp/_bazel_alexeagle/7de78b5a870ca0b5cd7d0aaac8b73a00/sandbox/darwin-sandbox/7/execroot/aspect_rules_ts/bazel-out/darwin_arm64-fastbuild/bin/examples/resolve_json_module/data.json' is not under 'rootDir' '/private/var/tmp/_bazel_alexeagle/7de78b5a870ca0b5cd7d0aaac8b73a00/sandbox/darwin-sandbox/7/execroot/aspect_rules_ts/bazel-out/darwin_arm64-fastbuild/bin/examples/resolve_json_module/bar'. 'rootDir' is expected to contain all source files.

This error doesn't happen for other files we got from _gather_declarations_from_js_providers because TypeScript doesn't try to emit anything for those. See https://github.com/microsoft/TypeScript/issues/38015#issuecomment-617433717

Possible to workaround?

Assuming no fix in tsc, we would need a way to provide a type-definition for data.json without the program containing an emittable file for it. Apparently there's a new feature in TypeScript 5.0 https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-0.html#allowarbitraryextensions Which lets you specify a data.d.json.ts file: https://github.com/microsoft/TypeScript/issues/52994 The problem with that is, we don't know how to produce such a file. I think that feature is intended for users to write such a thing by hand.

alexeagle commented 9 months ago

I think the fix we want to argue for (and likely send a PR) is that the "special case" mentioned in https://github.com/microsoft/TypeScript/issues/38015#issuecomment-617433717 should be extended. There are two ways I'd argue this:

A .json file is in the Program only because it is referenced in a type-only position, i.e. only reachable through import type. Then the .json file is excluded from emit. (Note, in the repro this isn't sufficient since the index.ts used the data as a value)
Simpler: if the .json file is outside the rootDir then the emit should be skipped rather than throw a TS6059. Avoid having to know whether it is used as a value.

@pyrocat101 I suspect we can patch this second behavior into your tsc.js binary pretty easily as a workaround.

gzm0 commented 9 months ago

TY for pinging me on this @alexeagle.

We have run into this issue as well. What works as a workaround for us, is to assign the JSON into an intermediate variable:

// foo/foo.ts
import jsonData from './data.json';
const data = jsonData;
export type Data = typeof data;

We are using this for the JSON values but it seems to work as well for types (verified on the i516 branch).

What is happening is that when compiling foo.ts typescript will actually dump the inferred type of jsonData into foo.d.ts:

-import data from './data.json';
+declare const data: {
+    a: string;
+}[];
 export declare const a: string;
 export declare type Data = typeof data;
+export {};

IDK why the export {}; appears, but most importantly, the declaration does not import the JSON anymore.

I'll have a closer look at the references @alexeagle posted. This does feel like a tsc issue to me, I'll attempt to reproduce this issue without bazel (just tsc -p <path>).

One thing that stands out for me in all the repros I've seen (not the one we have internally) is that bazel ts_project's and tsconfig.json's do not have a 1:1 mapping (and project references are not used). However, I'm unsure to what extent we should expect this to be an issue.

gzm0 commented 9 months ago

I can reproduce this issue without bazel:

Start at https://github.com/gzm0/rules_ts/tree/i516-isolated
bazel run @pnpm -- install --dir=$PWD/examples
cd examples/resolve_json_module
npx tsc -b bar

foo/index.ts:1:22 - error TS6307: File '/home/tos/gh/rules_ts/examples/resolve_json_module/foo/data.json' is not listed within the file list of project '/home/tos/gh/rules_ts/examples/resolve_json_module/foo/tsconfig.json'. Projects must list all files or use an 'include' pattern.

1 import jsonData from './data.json'
                       ~~~~~~~~~~~~~

Found 1 error.

It seems this is essentially an occurrence of https://github.com/microsoft/TypeScript/issues/25636

The workaround seems to be to simply configure all json files in include. Which IMHO is:

leaky: why would my project have to changes its input config if an upstream project includes a new file
not a good idea: json files are not included by default for good reason: There are many json files in a JS project that are not part of the source (tsconfig.json and package.json to just name 2). So including them by default is meh.

IIUC the situation correctly, IMHO the proper fix would be that typescript does not rely on JSON imports for types in its generated .d.ts files. (This is likely also better from an incremental compilation POV, since types on JSON have to be inferred).

alexeagle commented 9 months ago

Thanks @gzm0 - that workaround has the right shape IMO. Getting the type signature inlined (like --isolatedDeclarations proposal?) does improve incrementality too - if you change values in data.json but not types, it doesn't cause a cascading rebuild of ts_project that would have to take the .json as an input.

gzm0 commented 9 months ago

You mean https://github.com/microsoft/TypeScript/issues/47947 ? IIUC this would be huge for bazel, since we could avoid forwarding all declaration files of the dependencies.

pyrocat101 commented 9 months ago

Ah thanks. @gzm0's workaround works well enough for my use case.

aspect-build / rules_ts