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

"The inferred type of X cannot be named without a reference to Y" (TS2742) #159

Open
tinganho opened this issue Sep 28, 2022 · 16 comments
Open
Labels
documentation Improvements or additions to documentation

Comments

@tinganho
Copy link

npm_link_package labels in data and deps attributes in ts_project is causing typescript to issue portability errors.

Here is a reproduction and more info in the README:
https://github.com/tinganho/transitive-deps-bug

@cgrindel cgrindel added the bug Something isn't working label Sep 28, 2022
@cgrindel cgrindel added the discussion needed Discussion required to progress further label Oct 5, 2022
@jbedard
Copy link
Member

jbedard commented Oct 8, 2022

This might be an issue with symlinks and not only rules_ts? See microsoft/TypeScript#29808 (comment)

@cgrindel cgrindel added prioritized and removed discussion needed Discussion required to progress further labels Oct 13, 2022
@gregmagolan
Copy link
Member

gregmagolan commented Oct 18, 2022

Yup. Looks like this is a Typescript + symlinked node_modules structure issue and not specific to rules_js or Bazel.

Issue in TypeScript repo is very active: microsoft/TypeScript#47663

@mrmeku has a repro outside of Bazel with pnpm, which also uses a symlinked node_modules structure like rules_js: https://github.com/mrmeku/portable_types_repro (from microsoft/TypeScript#47663 (comment))

@gregmagolan gregmagolan added documentation Improvements or additions to documentation and removed prioritized bug Something isn't working labels Oct 18, 2022
@gregmagolan
Copy link
Member

One of the work-arounds in microsoft/TypeScript#47663 is to import type {} from "transitive": microsoft/TypeScript#47663 (comment).

I was able to get this reproduction to work by doing that for the transitive closure with this change:

$ git diff
diff --git a/packages/a/BUILD.bazel b/packages/a/BUILD.bazel
index 0744ca1..9be754c 100644
--- a/packages/a/BUILD.bazel
+++ b/packages/a/BUILD.bazel
@@ -5,5 +5,8 @@ package(default_visibility = ["//visibility:public"])
 ts_test(
     name = "a",
     srcs = ["foo.ts"],
-    deps = ["//packages/b:b"]
+    deps = [
+        "//packages/b:b",
+        "//packages/c:c",
+    ]
 )
\ No newline at end of file
diff --git a/packages/a/foo.ts b/packages/a/foo.ts
index faf0838..c227a19 100644
--- a/packages/a/foo.ts
+++ b/packages/a/foo.ts
@@ -1,4 +1,6 @@
 import { Foo } from '@myorg/b/bar';
+import type {} from '@myorg/b/bar';
+import type {} from '@myorg/c/qux';
 
 export interface Offline {
     pluginRegistry: Foo | undefined;
diff --git a/packages/b/bar.ts b/packages/b/bar.ts
index 44d3558..d8e7756 100644
--- a/packages/b/bar.ts
+++ b/packages/b/bar.ts
@@ -1,4 +1,5 @@
 import { Bar } from '@myorg/c/qux';
+import type {} from '@myorg/c/qux';
 
 export interface Foo {
     bar: Bar[];

@gregmagolan
Copy link
Member

Until the underlying problem with Typescript and the symlinked node_modules structure is fixed, a work-around will be needed.

@gregmagolan gregmagolan changed the title Transitive dependency are not linked underneath node_modules "The inferred type of X cannot be named without a reference to Y" (TS2742) Oct 19, 2022
@tinganho
Copy link
Author

tinganho commented Oct 19, 2022

Seems like indeed it is a TypeScript bug. Described more in here:

You get this error any time the declaration emitter can't synthesize a workable specifier for a module which it needs to name a type from. For example, if it appears that the only legal path is ../../other_module/foo via some file that's in <>/whatever, then that's not likely to work because the disk layout of the produced artifacts don't really have implicit dependencies on what peer directories of the output directory have.
The logic to synthesize these specifiers starts with the easy route of "Has this already been imported?", in which case re-use is easy and fine. Immediately past that lie many dragons and it's easy to get into a novel corner case where there is a speakable name to a module but TS just can't figure it out. Adding the import yourself is the easiest way to resolve the situation.

microsoft/TypeScript#48212 (comment)

I think the issue boils down to: transitive deps being resolved to outside the project?

@tinganho
Copy link
Author

tinganho commented Oct 27, 2022

@gregmagolan I had closer look at the the minimum reproducible project today. I noticed the symlink structure is not exactly described in PNPM:

Note, that under node_modules/.aspect_rules_js/@myorg+b@0.0.0/node_modules/@myorg the symlink c is not pointing to ../../@myorg+c@0.0.0/node_modules/@myorg/c as in PNPM docs:

image

The project is under sandbox/darwin-sandbox/3 but the symlink target is outside of the project subtree?

Note the c package is stored node_modules/.aspect_rules_js/@myorg+c@0.0.0/node_modules/@myorg/c, which the b package doesn't symlink to?

image

@tinganho
Copy link
Author

I'm thinking if the symlink target is being kept in the project's subtree. It might fix the issue?

@gregmagolan
Copy link
Member

gregmagolan commented Oct 27, 2022

That is an issue with Bazel itself wich controls how symlinks are layed out on disk. Historically the sandbox and runfiles trees created symlinks as absolute paths pointing out of the trees instead of relative within the trees.

However, this problem is now resolved in Bazel 6 with some help from the community. Bazel 6 is currently at 6.0.0rc1. If you use the latest rules_js with Bazel 6.0.0rc1 and set the --experimental_allow_unresolved_symlinks flag in your .bazelrc then the symlinks should be layed out on disk correctly by Bazel.

@tinganho
Copy link
Author

The bug still appears, although now inside the project subtree

The inferred type of 'Offline' cannot be named without a reference to '.aspect_rules_js/@myorg+c@0.0.0/node_modules/@myorg/c/qux'. This is likely not portable. A type annotation is necessary.

@flolu
Copy link

flolu commented Nov 28, 2022

@gregmagolan I'm still having this problem. And --allow_experimental_unresolves_symlinks is unrecognized by Bazel.

@gregmagolan gregmagolan removed their assignment Feb 5, 2023
@flolu
Copy link

flolu commented Feb 20, 2023

@gregmagolan could you provide more details for --allow_experimental_unresolves_symlinks? Adding it to .bazelrc doesn't fix the issue for me.

Is there any plan to get this fixed?

@gregmagolan
Copy link
Member

@flolu Bazel 6 the feature is on by default so it is only with Bazel 5 that --experimental_allow_unresolved_symlinks needs to be set explicitly. What version of Bazel are you using?

@flolu
Copy link

flolu commented Feb 20, 2023

@gregmagolan I'm using Bazel 6:

Bazelisk version: v1.13.2
Build label: 6.0.0
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Mon Dec 19 15:52:35 2022 (1671465155)
Build timestamp: 1671465155
Build timestamp as int: 1671465155

You can reproduce the issue like this:

git clone https://github.com/flolu/aspect-typescript
git checkout inferred-type-error
pnpm i
bazelisk test ...

It throws:

app/app.ts(4,7): error TS2742: The inferred type of 'app' cannot be named without a reference to '.aspect_rules_js/@types+express@4.17.17/node_modules/@types/express-serve-static-core'. This is likely not portable. A type annotation is necessary.

@gregmagolan
Copy link
Member

gregmagolan commented Feb 20, 2023

The underlying with Typescript and the symlinked node_modules structure is still open microsoft/TypeScript#47663, although it looks like the discussion is still active and someone has proposed a set of patches. The issue has also been reported on the pnpm repo 4 days ago pnpm/pnpm#6089 since pnpm also uses a symlinked node_modules structure.

For now a work-around is needed for this issue both with rules_js and pnpm. microsoft/TypeScript#47663 (comment) is one from the typescript issue and the reporter of the pnpm issue suggested one as well pnpm/pnpm#6089 (comment).

@flolu
Copy link

flolu commented Feb 20, 2023

@gregmagolan allright, thanks!

@Sayed-Helmy
Copy link

Same issue here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: On Deck
Development

No branches or pull requests

6 participants