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

No way to use composite project where outDir is determined by build tool #37378

Open
alexeagle opened this issue Mar 13, 2020 · 30 comments
Open
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@alexeagle
Copy link
Contributor

alexeagle commented Mar 13, 2020

In #37257 I discussed one way to host TS project references under Bazel, where each build step gets only .d.ts and tsconfig.json files from the referenced projects. This is broken because TS module resolution fails to locate the .d.ts files in their outDir.

In this issue, let's try a different approach for Bazel, to satisfy current TS module resolution, each build step should get the .ts sources of its referenced projects, and the .tsbuildinfo output as well.

Bazel is picky about the setting of --outDir, because it supports cross-compiled builds in distinct output locations. For example if you're on a Mac and building for local platform, the outDir is <workspace root>/bazel-out/darwin-fastbuild/bin. But if you're on a Mac and building for a docker image, you need native binaries to be for the target platform so outDir is <workspace root>/bazel-out/k8-fastbuild/bin and if you want binaries with debug information it's <workspace root>/bazel-out/k8-dbg/bin.

Since the outDir is platform-specific, we don't want to check in a config file that hard-codes it. So we prefer to always have Bazel pass the --outDir option on the command-line to tsc, rather than include it in the tsconfig files.

Now we come to the problem. TypeScript "composite" setting enables up-to-dateness checking in tsc. With this input structure, and trying to tsc -p b

a/a.ts
a/tsconfig.json (no outDir specified; a was compiled with tsc --outDir=bazel-out/darwin-fastbuild/bin/a)
b/b.ts
b/tsconfig.json
bazel-out/darwin-fastbuild/bin/a/a.d.ts
bazel-out/darwin-fastbuild/bin/a/tsconfig.tsbuildinfo

we get b/b.ts(1,17): error TS6305: Output file '/private/var/tmp/_bazel_alx/df60115ea7f2a64e10fb4aa64b7d827f/sandbox/darwin-sandbox/54/execroot/ts_composite/a/a.d.ts' has not been built from source file '/private/var/tmp/_bazel_alx/df60115ea7f2a64e10fb4aa64b7d827f/sandbox/darwin-sandbox/54/execroot/ts_composite/a/a.ts'.

This indicates that TS is looking for a.d.ts next to a.ts. If we hard-code the platform-dependent outDir into a/tsconfig.json like I've done here
https://github.com/alexeagle/ts_composite/pull/5 it solves this problem, but not in a way we can ship.

Note that the compilation of a/tsconfig.json produces a .tsbuildinfo file with a correct "outDir" (based on the --outDir flag that was passed to compilation of a

 "options": {
      "project": "../../../../a/tsconfig.json",
      "outDir": "./",
  }

So it seems like the behavior for compiling b is to read the outDir setting from a/tsconfig.json rather than trust the options.outDir reflected in the .tsbuildinfo output.

@alexeagle alexeagle changed the title --outDir command-line option not reflected in .tsbuildinfo --outDir command-line option in .tsbuildinfo not honored for composite project up-to-date check Mar 13, 2020
@alexeagle alexeagle changed the title --outDir command-line option in .tsbuildinfo not honored for composite project up-to-date check options.outDir in .tsbuildinfo not honored for composite project up-to-date check Mar 13, 2020
@sheetalkamat
Copy link
Member

when doing tsc -p b you are only building the project b and not a so there is no way to tell if a was built with extra options or not. Whatever is present in the config file of a is the only truth compiler can assume.

@alexeagle
Copy link
Contributor Author

Yes, I can see that. The only way tsc could do what I'm asking here is if the --outDir setting of b was considered to match a - that is, my output directory scheme always preserves the same directory structure as the sources, so if a was imported from ../a and b is compiled into --outDir=bazel-out/b then we could know that a may be in bazel-out/a

I think I should take @DanielRosenwasser up on a suggestion that we have a brief design discussion. After studying this problem for a week, it seems that Bazel's output directory is simply incompatible with typescript project references and one of them needs to change. Do you have a design meeting this week? Thanks!

@alexeagle
Copy link
Contributor Author

I hoped that --incremental false would disable this up-to-date checking (Bazel already does it) but I get

error TS6379: Composite projects may not disable incremental compilation.

is there any other way to avoid this checking?

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Mar 17, 2020
@RyanCavanaugh RyanCavanaugh self-assigned this Mar 17, 2020
@alexeagle alexeagle changed the title options.outDir in .tsbuildinfo not honored for composite project up-to-date check No way to use composite project where outDir is determined by build tool Mar 20, 2020
@alexeagle
Copy link
Contributor Author

@RyanCavanaugh thanks for the discussion today. I noted that you said TS repo itself is affected by this issue, requiring that you have multiple tsconfig files to express the difference between two different build options.

I have another proposal based on some more experimentation.

Take this scenario. We've already compiled lib and now want to compile app.

lib/
    tsconfig.json   // composite: true
bazel-out/mac/lib
    index.js
    index.d.ts
    tsconfig.tsbuildinfo
app/
    tsconfig.json   // "references": [  {"path": "../lib"}  ]}
    index.ts

tsc -p app --outDir bazel-out/mac/app

As we know with TS today, this fails because the reference goes to the lib/tsconfig which doesn't specify an outDir, so TS will look for lib/index.d.ts and not find it.

Let's say the semantics for references were expanded (maybe under some new flag) so that path: ../lib could be resolved relative to the outDir of the current build, meaning bazel-out/mac/app/../lib
(I think you suggested in the meeting that this makes sense to you)

This can work, but only if we also add a build step to copy the tsconfig file to the output directory - and we know that's complex because the paths need to be made relative to the new location.

So instead, since there is no tsconfig.json file in the output directory, we'd want this to resolve one step further to bazel-out/mac/lib/tsconfig.tsbuildinfo.

I could imagine that a reference to a tsbuildinfo file could be made to work in place of a reference to a tsconfig file? If so this seems simple and in my basic testing it works.

@alexeagle
Copy link
Contributor Author

#37509 shows the change to resolving reference paths that I'm currently using for prototyping. Along with a hack in Bazel to copy tsconfig into the outDir, this seems to work.

@alexeagle
Copy link
Contributor Author

@sheetalkamat FYI we shipped the new TS support for Bazel with a bunch of ugly caveats about this and related bugs
https://bazelbuild.github.io/rules_nodejs/TypeScript#ts_project

What do you or Ryan think of the proposal to resolve reference paths into the outDir?

@sheetalkamat
Copy link
Member

c:: @RyanCavanaugh @DanielRosenwasser
I am not sure if that's what we are looking for.. Resolving project reference from two different places means that (not only project but when processing dependencies and everything) I remember that design mentting we needed more general approach rather than solving just outDir issue?

@alexeagle
Copy link
Contributor Author

As that last reference points out, this problem has gotten worse. Bazel has more potential values for --outDir and it's really not feasible to require Bazel users to include all of these in the rootDirs of a downstream tsc project.

@RyanCavanaugh
Copy link
Member

Let's say the semantics for references were expanded (maybe under some new flag) so that path: ../lib could be resolved relative to the outDir of the current build, meaning

Daniel and I chatted about this for a while and I think this is sensible and very implementable. Basically just add a new flag (name TBD) where we compute the outDir of a referenced project as follows:

  • Compute the relative path from the referencing tsconfig to the referenced tsconfig
  • Apply that relative path to the outDir of the referenced project

This means that if you had

foo/
  tsconfig.json
  blah.ts
bar/
  tsconfig.json
  blub.ts
out1/
  foo/
    blah.d.ts
  bar/
    blub.d.ts
out2/
  foo/
    blah.d.ts
  bar/
    blub.d.ts

Then if foo references bar and foo's outDir is out1/foo, under this flag, we compute the path ../bar/, and instead of looking for bar/blub.d.ts, we look for out1/foo/../bar/blub.d.ts, which would resolve properly

Just to confirm: Would that solve your problem?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Feb 2, 2022

I think the key is that this flag only really makes sense when the entire source-tree is mirrored by a corresponding output tree. The above output format would work, and maybe would more specifically look like this for Bazel and similar systems:

projects/
├── src/
│   ├── foo
│   ├── bar
│   └── baz
├── win-out/
│   ├── foo
│   ├── bar
│   └── baz
├── mac-out/
│   ├── foo
│   ├── bar
│   └── baz
└── linux-out/
    ├── foo
    ├── bar
    └── baz

But it would not work for the following layout:

projects/
├── foo/
│   ├── src
│   ├── win-out
│   ├── mac-out
│   └── linux-out
├── bar/
│   ├── src
│   ├── win-out
│   ├── mac-out
│   └── linux-out
└── baz/
    ├── src
    ├── win-out
    ├── mac-out
    └── linux-out

So that's why we're curious whether it fits the bill for you.

@alexeagle
Copy link
Contributor Author

alexeagle commented Feb 2, 2022

Thanks for looking! That's very close.

Composite projects is the TS-idiomatic way to express one compilation unit which depends on the typings from another, and I have some examples which do that. However under Bazel it's not required to do it this way. Many users have a single tsconfig.json file at the root, and then configure bazel with a srcs attribute to limit which files are in the program. You could imagine that they run these commands:

% tsc --outDir out1 bar/blub.ts
# now we expect out1/bar/blub.d.ts to exist
% tsc --outDir out1 foo/blah.ts
# foo/blah.ts presumably has an import from ../bar/blub.ts
# it expects to resolve blub from out1/bar

so I think it would be better to design this such that having a tsconfig.json file in each compilation unit is not strictly required. In addition under Bazel it's possible to generate the tsconfig.json file, so that it lives in the output tree rather than the sources.

(If it's useful to you, a bunch of samples running tsc under Bazel are in https://github.com/bazelbuild/rules_nodejs/tree/stable/packages/typescript/test/ts_project)

@DanielRosenwasser you're correct, the first layout is what bazel uses (entire source tree structure is mirrored in each output folder). GN does the same, as one example. The second structure you describe isn't interesting from a bazel perspective.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Feb 2, 2022

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Feb 2, 2022

Composite projects is the TS-idiomatic way to express one compilation unit which depends on the typings from another, and I have some examples which do that. However under Bazel it's not required to do it this way. Many users have a single tsconfig.json file at the root, and then configure bazel with a srcs attribute to limit which files are in the program.

Okay, so I think the key to thinking about this is this: TypeScript project references roughly gives you two things

  1. builds that depend on other builds
  2. consuming output files rather than inputs

You want to be able to do (2) without setting up project references as well.

@alexeagle
Copy link
Contributor Author

Yes that's a good summary, Bazel can use a different mechanism than project references to accomplish 1, but our workarounds for 2 are not scaling to the number of potential outdirs.

I should note, another potential fix for our issue would be to allow the compilerOptions.rootDirs property to be set on the command line, using some way to encode its map-typed value. From the Bazel rule, we could easily set that to the current outDir.

@alexeagle
Copy link
Contributor Author

@RyanCavanaugh could it be as simple as:

  1. Under our new flag (I'll use --resolveFromOutDir)
  2. when encountering an import from ../foo/blah in a file bar/blub.ts
  3. we first use any output location of that file, say out1/bar/blub.d.ts (for this algorithm we could always assume --declaration if that's helpful to know an output location. we won't actually read or write to that path. out1/bar/blub.js would work too)
  4. and resolve the ../foo/blah import relative to that output location which would take us to out1/bar/../foo/blah.d.ts

@alexeagle
Copy link
Contributor Author

@RyanCavanaugh @DanielRosenwasser ping on this, should I be doing more to push design along? do you want a clumsy PR for it?

@RyanCavanaugh
Copy link
Member

@alexeagle we'll talk about it again this week, I think your suggestion is pretty sound. A PR would be nice as an anchoring point if you're willing

@alexeagle
Copy link
Contributor Author

Note, the proposal would also solve #22208 since out1/bar/../foo/blah.d.ts would be resolved before ../foo/blah.ts.

I'll try to work up a PR, any help would be appreciated to get something closer to landable :)

alexeagle added a commit to alexeagle/TypeScript that referenced this issue Mar 9, 2022
Proof of concept for resolving microsoft#37378

Under the new proposed `compilerOptions.resolveFromOutDir` boolean,
module resolution is attempted relative to the output folder.

This is analogous to loading from the rootDirs, however it allows
compilation where the output directory is configured on the command line
rather than in the tsconfig.json.

See the attached issue for context.

TODO:
- figure out what tests to add
- reason about whether this interacts correctly with other related
module resolution conditional logic
- verify this works in some Bazel projects where the problem is observed
gregmagolan pushed a commit to gregmagolan/TypeScript that referenced this issue Mar 10, 2022
Proof of concept for resolving microsoft#37378

Under the new proposed `compilerOptions.resolveFromOutDir` boolean,
module resolution is attempted relative to the output folder.

This is analogous to loading from the rootDirs, however it allows
compilation where the output directory is configured on the command line
rather than in the tsconfig.json.

See the attached issue for context.

TODO:
- figure out what tests to add
- reason about whether this interacts correctly with other related
module resolution conditional logic
- verify this works in some Bazel projects where the problem is observed
@alexeagle
Copy link
Contributor Author

@RyanCavanaugh the PR is up. I think if you accept this issue that will clue the bot into putting nicer labels on it :)

@sheetalkamat
Copy link
Member

sheetalkamat commented Mar 22, 2022

@alexeagle I think @RyanCavanaugh suggested changing outDir computation for the referenced project instead of module resolution. #48190 does changes to module resolution which is not what the suggestion was.

@alexeagle
Copy link
Contributor Author

Project References is just one use case that the PR fixes. As I explained above most Bazel users are simply compiling a project with tsc -p, and relying on earlier compilations to have populated the outDir with transitive typings.

#37378 (comment) is where I think @RyanCavanaugh agreed to the change in the module resolver.

@DanielRosenwasser
Copy link
Member

Yeah I think the idea of resolving modules relative to their outputs was what we agreed to in #48042.

@sheetalkamat
Copy link
Member

sheetalkamat commented Mar 22, 2022

But then that doesnt solve project reference issue... how do you know if the d.ts file you got is from project reference? there is definitely some confusion here since i was under impression that we want outDir for the project reference instead as i discussed this with @RyanCavanaugh this morning.
Not being able to see the file is from project reference will have impact on what the tsc -build does as well as editor experience.

@alexeagle
Copy link
Contributor Author

Could you be more specific what problem this introduces for tsc -build or language service users who enable the flag?

@alexeagle
Copy link
Contributor Author

Ping! Some large Bazel users are now on a fork of TypeScript to get this.

@sheetalkamat
Copy link
Member

Appologies that this slipped through other notifications..
tsc --build uses .d.ts ouptut checks of referenced project(Shared) to determine if project(App) needs to be completely rebuilt or just needs timestamps need update.. How is that suppose to work with change to only module resolution..

In editors, when say app project referecnes output d.ts of shared project through module resolution or any other way, we use its source file instead so we can walk through edits without having to save it.. This functionality might be affected with this change since we dont know resolved module is from project reference.

@gonzojive
Copy link

Ping! Some large Bazel users are now on a fork of TypeScript to get this.

Where is the fork code?

@alexeagle
Copy link
Contributor Author

Update: we now have better Bazel rules which don't require this at all. They copy all the sources and the node_modules tree to the bazel-out folder, then run tsc with that working directory, so there is a single root and TypeScript works properly as-is.

https://github.com/aspect-build/rules_ts

@rc-glean
Copy link

rc-glean commented Oct 10, 2022

@alexeagle when attempting to use a "references" key in a tsconfig.json using Aspect's rules_ts, I'm observing the error:

bazel build //typescript:compile_all_ts

ERROR: /Users/ray/private_repo/BUILD.bazel:251:11: Compiling TypeScript project //typescript:compile_all_ts [tsc -p typescript/tsconfig.json] failed: (Exit 2): tsc.sh failed: error executing command bazel-out/darwin-opt-exec-2B5CBBC6/bin/external/npm_typescript/tsc.sh --project typescript/tsconfig.json --outDir typescript --rootDir typescript

typescript/tsconfig.json(65,5): error TS6053: File '/private/var/tmp/_bazel_ray/0a45558874bc1f97bcaab4f86b774d47/sandbox/darwin-sandbox/9289/execroot/private_repo/bazel-out/darwin-fastbuild/bin/typescript/core' not found.

I'm noting a lack of tsconfig.json's with a references key in the examples with this rules_ts codesearch. Is it possible to use project references with rules_ts? Is there a workaround that hasn't made it into the examples yet?

@alexeagle
Copy link
Contributor Author

alexeagle commented Oct 10, 2022

@rc-glean it's probably better to file that on the rules_ts repo than on TypeScript, I don't think there's a bug in this repo

edit: I see aspect-build/rules_ts#163 now - when you cross-post an issue to more than one repo, it's good practice to link to the other one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants