Rules_nodejs: type inference: ts_library behaves differently than tsc

[stephanwlee]

🐞 bug report

Affected Rule

ts_library

Is this a regression?

Uncertain if this is a regression.

Description

When using ts_library on very simple module that imports and make use of a method in ngrx, tsc fails to infer the return type. This contrasts with the vanilla tsc which successfully infers the type and builds the file.

🔬 Minimal Reproduction

https://github.com/stephanwlee/bazel-ngc-tsc-bad

Repro steps are detailed in the README.
Note that I am using Bazel 0.26.1.

🔥 Exception or Error

bad-ngc/BUILD.bazel:3:1: Compiling TypeScript (devmode) //:bug failed (Exit 1) tsc_wrapped failed: error executing command bazel-out/host/bin/external/npm/@bazel/typescript/bin/tsc_wrapped @@bazel-out/k8-fastbuild/bin/bug_es5_tsconfig.json

Use --sandbox_debug to see verbose messages from the sandbox
src/core.actions.ts:17:14 - error TS2742: The inferred type of 'meow' cannot be named without a reference to '../external/npm/node_modules/@ngrx/store/src/models'. This is likely not portable. A type annotation is necessary.

17 export const meow = createAction('[Core] wow');
                ~~~~
src/core.actions.ts:17:14 - error TS2742: The inferred type of 'meow' cannot be named without a reference to '../external/npm/node_modules/@ngrx/store/store'. This is likely not portable. A type annotation is necessary.

17 export const meow = createAction('[Core] wow');
                ~~~~
src/core.reducers.ts:52:14 - error TS2742: The inferred type of 'getActivePlugin' cannot be named without a reference to '../external/npm/node_modules/@ngrx/store/src/selector'. This is likely not portable. A type annotation is necessary.

52 export const getActivePlugin = createSelector(
                ~~~~~~~~~~~~~~~
src/core.reducers.ts:52:14 - error TS2742: The inferred type of 'getActivePlugin' cannot be named without a reference to '../external/npm/node_modules/@ngrx/store/store'. This is likely not portable. A type annotation is necessary.

52 export const getActivePlugin = createSelector(
                ~~~~~~~~~~~~~~~

Target //:bug failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 11.643s, Critical Path: 3.50s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

## 🌍 Your Environment **Operating System:**

  
DISTRIB_DESCRIPTION="Debian GNU/Linux rodete (upgraded from: Ubuntu 14.04.5 LTS)"
  

Output of bazel version:

  
Build label: 0.26.1
Build time: Thu Jun 6 11:05:05 2019 (1559819105)
Build timestamp: 1559819105
Build timestamp as int: 1559819105
  

Rules version (SHA):

  
https://github.com/bazelbuild/rules_nodejs/releases/download/0.34.0/rules_nodejs-0.34.0.tar.gz
  

Anything else relevant?
I poked around tsc and tsc_wrapped and inspected which files are being traversed (printed modules that are being resolved by compilerHost). Nothing was out of ordinary for both versions -- i.e., tsc_wrapped also traversed through the typed dependency of @ngrx/store (store/typing.d.ts, store/index.d.ts, ... store/src/model.d.ts)

[a-ignatov-parc]

There are some issues reported in TS repo, so it's hard to say exactly where the problem is

• https://github.com/microsoft/TypeScript/issues/29221
• https://github.com/microsoft/TypeScript/issues/30858

Unfortunately, we had the same problems in our code base. We test some possible fixes like adding paths to modules with there own typings that came not from @types to typeRoots, but we didn't succeed.

For now, we use workarounds with importing modules and adding types with typeof.

[pauldraper]

Encountering the same problem.

I'm guessing the issue is ts_library's "path": { "*": ... }.

Rather than using the traditional nodes_modules file layout, it uses path mappings to effectively use a different node_modules. And TypeScript gets confused by how to add imports that reference those files.

[pauldraper]

I also created repro here: https://github.com/pauldraper/ts-bazel-infer-import

The issue is that when generating import() for the declarations, tsc is (1) preferring a relative path and (2) erroring if the path includes node_modules (which it does; it's some ../external/npm/node_modules type path).

[pauldraper]

Additional info: Beginning in TS 3.2, tsc started checking the generated path for node_modules and failing if it found it.

Either removing that check, or making the inferred import prefer non-relative paths would fix this issue.

[alexeagle]

Note, one possible "escape hatch" is to use plain tsc rather than ts_library, see https://github.com/bazelbuild/rules_nodejs/blob/master/packages/typescript/docs/install.md#alternatives

[tony-scio]

@alexeagle I don't really want to eject from ts_library because then we'd lose some benefits like keeping the worker running.

To workaround it, sometimes have to specify function definitions twice. Like:

export const handleRequestError: (requestType: string, response: Response, auth?: AuthState) => void = (requestType: string, response: Response, auth?: AuthState) => {
  ...
}

Would definitely be nice to see this fixed so the duplication can be removed. Please let me know if there's any useful info I could provide.

[eordano]

Just stumbled upon this issue. It would be nice to see some fix. Maybe, a workaround would be to have the ability to set importHelpers in the tsconfig file from the ts_library rule.

[pope]

For now, we use workarounds with importing modules and adding types with typeof.

Might you be able to go into more details about what that looks like?

[a-ignatov-parc]

@pope sure!

For example, we have

import { welcomeUser, blockUser, unblockUser } from '../actions';

const mapDispatchToProps = {
    welcomeUser,
    unblockUser,
    blockUser,
};

Which fails to build with

/account-action-column.tsx:70:7 - error TS2742: The inferred type of 'mapDispatchToProps' cannot be named without a reference to '../../../../../../../../../external/npm/node_modules/redux'. This is likely not portable. A type annotation is necessary.

70 const mapDispatchToProps = {
         ~~~~~~~~~~~~~~~~~~

This problem can be fixed in two ways:

1. Explicitly define mapDispatchToProps type:
   ```ts
   import { welcomeUser, blockUser, unblockUser } from '../actions';

const mapDispatchToProps: {
    welcomeUser: typeof welcomeUser;
    unblockUser: typeof unblockUser;
    blockUser: typeof blockUser;
} = {
    welcomeUser,
    unblockUser,
    blockUser,
};
```

1. Add explicit redux import:
   ```ts
   import _ from 'redux'; // Explicit import
   import { welcomeUser, blockUser, unblockUser } from '../actions';

const mapDispatchToProps = {
    welcomeUser,
    unblockUser,
    blockUser,
};
```
> Also, you will be require to add `@npm//redux` label to `deps` attribute.

[flolu]

Another _abosulutely ugly_ way to prevent the error is by setting the type of the affected object to any.

Is there any progress on this issue?
I am encountering the problems when using @ngrx/store and @ngrx/effects

[flolu]

For example I get 3 errors from my auth.effects.ts:

The inferred type of 'login$' cannot be named without a reference to '../../../../../external/npm/node_modules/@ngrx/effects/effects'
The inferred type of 'login$' cannot be named without a reference to '../../../../../external/npm/node_modules/rxjs'
The inferred type of 'login$' cannot be named without a reference to '../../../../../external/npm/node_modules/@ngrx/store/src/models'

and importing those 3 modules into auth.effects.ts fixes the errors:

import * as __ngrxEffectsTypes from '@ngrx/effects/src/models';
import * as __ngrxStoreTypes from '@ngrx/store/src/models';
import * as __rxjsTypes from 'rxjs';

Is it possible to import those modules once, e.g. in app.module.ts? I would be ok with this workaround when I wouldn't have to import those modules in every file I use ngrx

[Toxicable]

@flolu Those errors there you can solve by adding a explicity (rather than implicity) type to whatever is throwing the error.
eg: before

getPerson() { //return type is inferred to be Observable<Person>
  return this.person$; // where this.person is a Observable<Person>
}

replace with

getPerson(): Observable<Person> {
  return this.person$;
}

This is an actual a tsc error, but it's not a bug. Since with ts_library you usually compile your app in smaller chunks, so in order for another target to use the types from another they need to have those references explicity referenced in certain palces.
If you compiled all of your code in a single pass this wouldn't be an issue.

[flolu]

@Toxicable that is tolerable for your example. But say for example you want to create an NgRx action. It would like like this:

const login: ActionCreator<
  string,
  (props: { email: string }) => { email: string } & TypedAction<string>
> = createAction(`${PREFIX} ${LOGIN}`, props<{ email: string }>());

...and I really don't want to annotate hundreds of actions with a multi-line type, which would usually be created automatically :disappointed:

So adding all explicit types would be really painful to maintain.

If you compiled all of your code in a single pass this wouldn't be an issue.

Is it possible to achieve this with ts_library + ng_module?

[alexeagle]

The root cause is that ts_library is the google-internal rule for building typescript, and Google doesn't have any node_modules directories at all - all third-party libraries are vendored into the monorepo.
This exposes a bug based on an org chart. I don't work at Google anymore, and the ts_library rule has its source-of-truth in google codebase. We are having a slow time merging PRs to rules_typescript because the teams there are told to prioritize their internal customers, and have no real drive to make them work for external customers too. Same state as rules_closure.
One of our motivations for introducing ts_project is to mitigate the risk of depending on Google to do maintenance and bugfixes for parts of the rules they don't depend on themselves.
At its core, ts_library has to take a less breaking approach to laying out the dependencies such that tricks like pathmapping in the tsconfig aren't required. Otherwise there's no way to fix these compatibility pitfalls.

[flolu]

@alexeagle Ok that's not so good news. Will ts_project be a replacements for ts_library at some point?

I am also not so sure why I am only encountering this problem with Angular. For example I use ts_library for some Node.js services and I have never had similar problems there (although using many external packages).

One more question: Is it possible to suppress the errors as they don't seem to take any effect on the running application, maybe we can just ignore the errors?

[ACollectionOfAtoms]

Have been running into this issue a lot with styled-components.

Mostly with stuff like

const css = css`
//
`

and

const Comp = styled(C)`
//
`

we've been solving these with import * as _ from 'styled-components which seems _fine_ but not ideal of course.

[flolu]

I am now using a workaround for this issue by @joeljeske

For Typescript 3.8.3 it goes like this:

1. Add a typescript+3.8.3.patch file to your projects' /patches directory

diff --git a/node_modules/typescript/lib/typescript.js b/node_modules/typescript/lib/typescript.js
index 583f65b..f247520 100644
--- a/node_modules/typescript/lib/typescript.js
+++ b/node_modules/typescript/lib/typescript.js
@@ -90563,6 +90563,7 @@ var ts;
     }
     ts.isInternalDeclaration = isInternalDeclaration;
     var declarationEmitNodeBuilderFlags = 1024 /* MultilineObjectLiterals */ |
+        67108864 /* AllowNodeModulesRelativePaths */ |
         2048 /* WriteClassExpressionAsTypeLiteral */ |
         4096 /* UseTypeOfFunction */ |
         8 /* UseStructuralFallback */ |

1. Install patch-package

yarn add -D patch-package

1. Add it to the postinstall script of your package.json

{
  "scripts": {
    "postinstall": "patch-package"
  },
}

I am not sure if using this patch has any critical downsides? @alexeagle

But I can confirm that it "fixes" the issue.

[joeljeske]

@flolu glad that worked for you! Would you mind posting your thoughts on the upstream ticket I created (https://github.com/microsoft/TypeScript/issues/37960) so that we can get an official option built in to TS and not require a patch?

[flolu]

@joeljeske Yeah sure I will post something, that the issue will get more attention. But honestly, I don't really understand what the underlying issue is. So I can't talk about the details.

All I know is that it is something I would appreciate being implemented 😂

[joeljeske]

The issue is that Typescript does not think that it is allowed add an import to "react" or "rxjs" or some libs in node_module if it needs to because node_modules is in a different place. It ius only aware of how to add relative paths to the node_module directory. Typically, when you create .d.ts file, they are to be used in a separate repo and therefore cannot rely on relative paths to the node_module directory and the relative path may be incorrect.

In my case (as explained in the upstream TS ticket microsoft/TypeScript#37960), I am never shipping .d.ts files for use outside of the repo, they are only used internally in the build as a linking mechanism between each ts_library package. This means that I should be allowed to expressly tell TS that I am ok with relative paths in my .d.ts file since I know they will always result in valid paths.

If you were planning to ship your .d.ts files to NPM for example, this patch or this fix should not be used as it will result in invalid .d.ts files for your consumers. The real fix for that scenario is for ts_library to not put node_modules in a unique locations as @alexeagle mentioned in his last comment.

[matthewjh]

@joeljeske @alexeagle

I'm encountering this issue as I try to migrate our large Angular monorepo to use Bazel, using ts_library. Explicitly adding the inferrable types will be a royal pain. Do you know whether there are any other workarounds? Is there some way we can re-jigg our node_module dependencies to work around this problem?

I checked out the angular repository from GitHub and was able to recreate the same problem there by adding a field like x$ = defer(() => of(5)), which astounds me.

@alexeagle you mentioned that Google uses vendored 3rd party dependencies, therefore this particular issue does not arise. What does that look like? How would we do that? Pull in our 3rd pary deps via http_archive or local repository and then what? I appreciate any help or guidance you can provide.

[joeljeske]

As mentioned in an earlier comment, I am using a tiny patch on typescript directly to disable this behavior. It will only work out safely if you do not work publish the dts files explicitly anywhere.

https://github.com/microsoft/TypeScript/issues/37960

[flolu]

@matthewjh here is the patch for the latest Typescript version (3.9.6):

diff --git a/node_modules/typescript/lib/typescript.js b/node_modules/typescript/lib/typescript.js
index 73d31bc..b455995 100644
--- a/node_modules/typescript/lib/typescript.js
+++ b/node_modules/typescript/lib/typescript.js
@@ -92621,6 +92621,7 @@ var ts;
     }
     ts.isInternalDeclaration = isInternalDeclaration;
     var declarationEmitNodeBuilderFlags = 1024 /* MultilineObjectLiterals */ |
+        67108864 /* AllowNodeModulesRelativePaths */ |
         2048 /* WriteClassExpressionAsTypeLiteral */ |
         4096 /* UseTypeOfFunction */ |
         8 /* UseStructuralFallback */ |

Just:

1. put this file named typescript+3.9.6.patch inside your projects ~root/patches directory
2. install patch-package
3. add a postinstall script to your package.json: "postinstall": "patch-package --patch-dir ./patches"

[noosxe]

Reading through all the comments and also some discussions that were going in https://github.com/microsoft/TypeScript/issues/37960 I can definitely see that it is not something that I can do. In my case typescript sources are generated and not hand written and we intend to ship a built npm package that will contain js code and .d.ts types. I still can't wrap my head around the right way to go about this.

[matthewjh]

Thanks @flolu and @joeljeske. That worked. Such a shame that we need to do it. I'll put a comment on the Typescript issue -- we really need to make sure it moves forward.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[tony-scio]

I can confirm this is still an issue and very inconvenient. Hopefully it doesn't auto close.