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

BUILD file generation doesn't understand the @types convention for scoped packages #1033

Closed
alexeagle opened this issue Aug 19, 2019 · 5 comments
Assignees
Labels
Milestone

Comments

@alexeagle
Copy link
Collaborator

Context from Angular slack: https://angular-team.slack.com/archives/C4WCRN728/p1565947484085800

Pete Bacon Darwin
Does anyone have any ideas why this is failing: https://circleci.com/gh/angular/angular/425302 and how to fix it?
You’ll notice that the packages/localize/inlining:inlining build is failing because it cannot find the type declaration for @babel/core. (edited)
But I have installed the @types/babel__core typings in npm and inside VS Code it is able to find the typings correctly.
I think it might be related to the mapping of namespace in the package name to __ in the DefinitelyTyped package. I.E. @babel/core => babel__core.
I see in the generated tsconfig.json for the rule there is the following paths entry:

    "paths": {
      ...
      "*": [
        "external/npm/node_modules/*",
        "external/npm/node_modules/@types/*"
      ],
      ...

I wonder if this explicit path is causing TS in the ts_library rule to only try external/npm/node_modules/@types/@babel/core, which obviously doesn’t exist?

From looking at this: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master#what-about-scoped-packages
I think we need to add this to the paths.

"@babel/core": [
        "babel__core"
]

Is that possible with ts_library?

Alex Eagle 9:21 AM
interesting, I had no idea they did this kind of transform. we should be able to easily do this automatically in tsconfig.bzl
your workaround is probably to patch a PR
somewhere around here https://github.com/bazelbuild/rules_typescript/blob/master/internal/common/tsconfig.bzl#L119

@alexeagle
Copy link
Collaborator Author

I made a minimal repro in a fresh dir without Bazel involved:

$ yarn init -y
$ yarn add [email protected]    # Also repros at 3.5.3 and 3.7.0-dev.20191014
$ yarn add @types/node @types/babel__traverse
$ mkdir my_types
$ mv node_modules/@types/* my_types

tsconfig.json looks like

{
    "compilerOptions": {
        "noImplicitAny": true,
        "noEmitOnError": true,
        "traceResolution": true,
        "typeRoots": ["./my_types"]
    },
    "files": [
        "file.ts",
        "my_types/babel__traverse/index.d.ts",
        
    ]
}

Then a simple file.ts to test it:

import {NodePath} from '@babel/traverse';
import * as fs from 'fs';
console.error(Object.keys(NodePath));
console.error(fs.readdirSync('.'));

The "fs" import works, but the @babel/traverse doesn't.

Interestingly this appears in the output:

$ ./node_modules/.bin/tsc
======== Resolving type reference directive 'babel__traverse', containing file '/usr/local/google/home/alexeagle/Projects/repro1033/__inferred type names__.ts', root directory '/usr/local/google/home/alexeagle/Projects/repro1033/my_types'. ========
Resolving with primary search path '/usr/local/google/home/alexeagle/Projects/repro1033/my_types'.
Found 'package.json' at '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/package.json'.
'package.json' has 'types' field 'index' that references '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/index'.
File '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/index' does not exist.
Loading module as file / folder, candidate module location '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/index', target file type 'TypeScript'.
File '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/index.d.ts' exist - use it as a name resolution result.
Resolving real path for '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/index.d.ts', result '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/index.d.ts'.

======== Type reference directive 'babel__traverse' was successfully resolved to '/usr/local/google/home/alexeagle/Projects/repro1033/my_types/babel__traverse/index.d.ts' with Package ID '@types/babel__traverse/[email protected]', primary: true. ========

...

file.ts:1:24 - error TS7016: Could not find a declaration file for module '@babel/traverse'. '/usr/local/google/home/alexeagle/Projects/repro1033/node_modules/@babel/traverse/lib/index.js' implicitly has an 'any' type.
  Try `npm install @types/babel__traverse` if it exists or add a new declaration (.d.ts) file containing `declare module '@babel/traverse';`

Doesn't actually seem to matter if my_types/babel__traverse/index.d.ts is in the program (listed in files) or not.

It seems like the typeRoots feature doesn't interact properly with Scoped package detected, looking in 'babel__traverse' mechanism for discovering the Babel typing, even though the node typing works.

@alexeagle
Copy link
Collaborator Author

microsoft/TypeScript#30599 describes how typeRoots doesn't actually affect the module resolution.

The workaround suggested there does work wrap the content of my_types/babel__traverse/index.d.ts in

declare module "@babel/traverse" {
...
}

I think that's the most straightforward way to get this to work under ts_library where we zero out the types field. (Another answer would be to use plain tsc in this location but I think that's even more muddy to mix ways of running the compiler)

alexeagle added a commit to alexeagle/angular that referenced this issue Oct 15, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033
alexeagle added a commit to alexeagle/angular that referenced this issue Oct 15, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033
matsko pushed a commit to angular/angular that referenced this issue Oct 16, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033

PR Close #33176
@alexeagle
Copy link
Collaborator Author

This isn't a bazel/rules_nodejs bug, it's just an interaction of TypeScript with the typeRoots feature that we use. angular/angular#33176 shows how Angular worked around it.

petebacondarwin pushed a commit to petebacondarwin/angular that referenced this issue Oct 17, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033
petebacondarwin pushed a commit to petebacondarwin/angular that referenced this issue Oct 17, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033
matsko pushed a commit to angular/angular that referenced this issue Oct 17, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033

PR Close #33226
ODAVING pushed a commit to ODAVING/angular that referenced this issue Oct 18, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033

PR Close angular#33176
ODAVING pushed a commit to ODAVING/angular that referenced this issue Oct 18, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033

PR Close angular#33226
AndrusGerman pushed a commit to AndrusGerman/angular that referenced this issue Oct 22, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033

PR Close angular#33176
AndrusGerman pushed a commit to AndrusGerman/angular that referenced this issue Oct 22, 2019
These were getting included in the @angular/localize package.
Instead, patch the upstream files to work with TS typeRoots option

See bazel-contrib/rules_nodejs#1033

PR Close angular#33226
@filipesilva
Copy link
Contributor

@alexeagle also seeing this while migrating the CLI repo to Bazel. Patching node_modules contents (what was done here) doesn't really sound like it should be the answer here.

The typeRoots interaction might be unexpected but it's not part of the user configuration so it's not as if a rules_typescript user is opting into the weirdness. It's something that rules_typescript adds to make the setup work. microsoft/TypeScript#30599 (comment) makes it sound like this isn't a TS bug at all either, but rather that typeRoots isn't intended to have anything to do with module resolution to begin with.

I don't really understand why the issue was closed. It's not a typescript bug, it's not user error, it's not a problem without rules_typescript, and there's no workaround besides "use different dependencies or change them". It really sounds like it's a bug in rules_typescript use of typescript.

@filipesilva
Copy link
Contributor

Here's a more ergonomic approach: make a typings.d.ts (or whatever name you want) and declare modules with re-exports for the problematic typings:

declare module '@babel/core' {
  export * from '@types/babel__core';
}
declare module '@babel/generator' {
  export { default } from '@types/babel__generator';
}
declare module '@babel/traverse' {
  export { default } from '@types/babel__traverse';
}
declare module '@babel/template' {
  export { default } from '@types/babel__template';
}

This might need TS 3.8 for https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#export-star-as-namespace-syntax.

filipesilva pushed a commit to angular/angular-cli that referenced this issue Oct 14, 2020
…s.d.ts` during bazel builds

`packages/angular_devkit/build_angular/src/typings.d.ts` is only needed for a Bazel build to workaround bazel-contrib/rules_nodejs#1033

Including this typings in a non Bazel build creates broken triple slash references https://unpkg.com/browse/@angular-devkit/[email protected]/src/utils/process-bundle.d.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants