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

foo.ts is resolved before foo.d.ts even if the latter is in files[] #22208

Open
alexeagle opened this issue Feb 27, 2018 · 10 comments
Open

foo.ts is resolved before foo.d.ts even if the latter is in files[] #22208

alexeagle opened this issue Feb 27, 2018 · 10 comments
Labels
Committed The team has roadmapped this issue Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@alexeagle
Copy link
Contributor

alexeagle commented Feb 27, 2018

Affects at least TS 2.7.2 and 2.6.2. This problem appears when compiling files separately, like we do under Bazel.

Imagine this simple app

src/lib.ts

export const a = 1;

src/main.ts

import {a} from './lib';

Imagine lib.ts was compiled separately, so there already exists
dist/lib.d.ts

export declare const a = 1;

Now, I want to compile main.ts as a separate program. Given src/tsconfig.json

{
  "compilerOptions": {
    "rootDirs": [
      ".",
      "../dist"
    ],
    "outDir": "../dist",
    "declaration": true
  },

  "files": [
    "main.ts",
    // lib was compiled separately
    "../dist/lib.d.ts"
  ]
}

We see that lib.d.ts is already in the program before resolution begins. However the compiler resolves the import statement to the lib.ts file instead, adding it to the program, and tries to emit on top of an input, so the error is

$ ./node_modules/.bin/tsc -p src
error TS5055: Cannot write file '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/dist/lib.d.ts' because it would overwrite input file.

Okay, we didn't want lib.ts in the program, so we should just use --noResolve to prevent that, but it's also broken:

$ ./node_modules/.bin/tsc -p src --noResolve --traceResolution
======== Resolving module './lib' from '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/main.ts'. ========
Module resolution kind is not specified, using 'NodeJs'.
'rootDirs' option is set, using it to resolve relative module name './lib'.
Checking if '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/' is the longest matching prefix for '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/lib' - 'true'.
Checking if '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/dist/' is the longest matching prefix for '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/lib' - 'false'.
Longest matching prefix for '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/lib' is '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/'.
Loading 'lib' from the root dir '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/', candidate location '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/lib'.
Loading module as file / folder, candidate module location '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/lib', target file type 'TypeScript'.
File '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/lib.ts' exist - use it as a name resolution result.
======== Module name './lib' was successfully resolved to '/usr/local/google/home/alexeagle/Projects/repro_ts_resolving_ts/src/lib.ts'. ========
src/main.ts(1,17): error TS2307: Cannot find module './lib'.

So far our workaround at Google is one of:

  • use Bazel sandboxing to make the inputs appear in different places
  • use our own custom compiler which elides any emit to files we don't expect

However under Bazel we sometimes cannot sandbox (eg. on Windows) and cannot use our custom compiler (eg. when we are compiling it)
so we are stuck.

I found that ts.CompilerOptions.suppressOutputPathCheck could be a workaround, but tsc doesn't allow that flag from the command line or tsconfig.json (it's not in optionDeclarations in commandLineParser.ts)

@alexeagle
Copy link
Contributor Author

By the way, the previous compilation produced a readonly lib.d.ts so even if I add suppressOutputPathCheck the compilation still fails with EPERM: operation not permitted

@mhegazy
Copy link
Contributor

mhegazy commented Feb 27, 2018

the rootDirs resolution logic gives precedence to closest directory. the rational is that these rootDirs are meant to "augment" and not "overwrite" the contents of local files. this saves us from looking ./a in all of the rootDirs until we find it in the local folder, we always look in the local folder first, since we are likely to find it.

that said. i would like to get more context on what you are trying to achieve. this seems similar to the Project to project reference @RyanCavanaugh is working on.

@alexeagle
Copy link
Contributor Author

Just trying to compile parts of an application independently, the same thing we've been doing at Google from the beginning. But as I mentioned, we've worked around this problem with bazel sandboxing and our custom compiler. Now I want to use tsc to compile our custom compiler itself, without relying on sandboxing as that's broken on Windows.

Yes, the Bazel approach is a precursor to TS natively understanding multiple compilation units, I'd be happy to discuss that!

In the meantime I still haven't found a workaround for this issue...

@evmar
Copy link
Contributor

evmar commented Mar 1, 2018

My attempt at restating Alex's problem: given that compilation already happened for his 'lib' thinger, his tsconfig explicitly lists a .d.ts file. tsc shouldn't be looking around to find an extra .ts file and parse+emitting for it in that case because it's doing extra work -- the whole reason he has this lib.d.ts ready around is because he built it in parallel.

alexeagle added a commit to bazelbuild/rules_typescript that referenced this issue Mar 1, 2018
When building outside the Bazel sandbox (in particular, on Windows where the sandbox is not available),
vanilla tsc doesn't support multiple compilation units. See details in that TypeScript bug.

The workaround is to build tsc_wrapped as a single compilation unit.

PiperOrigin-RevId: 187475876
@mhegazy
Copy link
Contributor

mhegazy commented Mar 1, 2018

given that compilation already happened for his 'lib' thinger, his tsconfig explicitly lists a .d.ts file. tsc shouldn't be looking around to find an extra .ts file and parse+emitting for it in that case because it's doing extra work

These are really two different phases. the first is identifing what files belong to the compilation unit by looking at imports. this lists all files. which includes src/main.ts and dist/lib.d.ts, then the compiler expands the set of source files by following imports and triple-slash references. at this point it runs into an import for ./lib, and tries to resolve it, and that leads to a new file src/lib.ts there is not really a place that the compiler thinks that src/lib.ts and dist/lib.d.ts are the same file.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 1, 2018

As i said earlier, the alternative is to go through sourceDirs as they are listed, but that means that for the common case, we will be touching the disk many times for files that do not exist. this optimization of finding the shortest path allows us to lookup files in the same folder as they were referenced before trying other rootDirs.

@alexeagle
Copy link
Contributor Author

I think the way to fix this is not to always try to resolve imports using other rootDirs, only to check if the rootFiles already satisfy an import.

@mprobst
Copy link
Contributor

mprobst commented Mar 28, 2018

Alternative idea: when --noResolve is true, just never look at the file system and only use the list of source files as the source of truth on what files are present? Then in the example above, ../dist/lib.ts would consistently not be found, and compilation would succeed by finding lib.d.ts (or fail if that's not around).

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Mar 28, 2018
alexeagle added a commit to bazelbuild/rules_typescript that referenced this issue Apr 26, 2018
As indicated in this comment, we need to allow TS to read files like node_modules/foo/package.json, and this cannot be included in the files[] list in tsconfig, so we still cannot restrict fileExists to only files in the cache. Ultimately as Martin points out this is the correct behavior.

This fix is needed in Angular, where we compile rxjs in an aspect to get the "ESM5" JavaScript flavor, and this suffers from microsoft/TypeScript#22208 where we try to read a `.ts` file when the `.d.ts` file was in the root files. In this case we don't have a worker mode so the usual sandboxing doesn't mask the problem.

PiperOrigin-RevId: 194095485
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Oct 9, 2018
@RyanCavanaugh RyanCavanaugh added this to the Future milestone Oct 9, 2018
@rkirov
Copy link
Contributor

rkirov commented Mar 28, 2020

@RyanCavanaugh just to confirm on the "help wanted" and "committed" tags that is fixing it the way #22208 (comment) is proposing?

@manughub
Copy link

manughub commented Apr 8, 2020

Here's a reproduction of the issue.

alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Oct 17, 2020
When building outside the Bazel sandbox (in particular, on Windows where the sandbox is not available),
vanilla tsc doesn't support multiple compilation units. See details in that TypeScript bug.

The workaround is to build tsc_wrapped as a single compilation unit.

PiperOrigin-RevId: 187475876
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Oct 17, 2020
As indicated in this comment, we need to allow TS to read files like node_modules/foo/package.json, and this cannot be included in the files[] list in tsconfig, so we still cannot restrict fileExists to only files in the cache. Ultimately as Martin points out this is the correct behavior.

This fix is needed in Angular, where we compile rxjs in an aspect to get the "ESM5" JavaScript flavor, and this suffers from microsoft/TypeScript#22208 where we try to read a `.ts` file when the `.d.ts` file was in the root files. In this case we don't have a worker mode so the usual sandboxing doesn't mask the problem.

PiperOrigin-RevId: 194095485
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Oct 18, 2020
When building outside the Bazel sandbox (in particular, on Windows where the sandbox is not available),
vanilla tsc doesn't support multiple compilation units. See details in that TypeScript bug.

The workaround is to build tsc_wrapped as a single compilation unit.

PiperOrigin-RevId: 187475876
alexeagle added a commit to alexeagle/rules_nodejs that referenced this issue Oct 18, 2020
As indicated in this comment, we need to allow TS to read files like node_modules/foo/package.json, and this cannot be included in the files[] list in tsconfig, so we still cannot restrict fileExists to only files in the cache. Ultimately as Martin points out this is the correct behavior.

This fix is needed in Angular, where we compile rxjs in an aspect to get the "ESM5" JavaScript flavor, and this suffers from microsoft/TypeScript#22208 where we try to read a `.ts` file when the `.d.ts` file was in the root files. In this case we don't have a worker mode so the usual sandboxing doesn't mask the problem.

PiperOrigin-RevId: 194095485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants