Skip to content

ts_project_worker: default-initialize tsc outDir #812

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ts_project_worker: default-initialize tsc outDir #812

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ts/private/ts_project_worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ function createProgram(args, inputs, output, exit) {
}

function applySyntheticOutPaths() {
host.optionsToExtend.outDir = `${host.optionsToExtend.outDir}/${SYNTHETIC_OUTDIR}`;
host.optionsToExtend.outDir = `${host.optionsToExtend.outDir || host.optionsToExtend.rootDir || '.'}/${SYNTHETIC_OUTDIR}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use rootDir? That seems wrong imo... 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is this rootDir different then compilerOptions.rootDir?

Copy link
Contributor Author

@malt3 malt3 Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cwd of the worker is the Bazel bindir.
rootDir is set to the package directory we are currently in by default, so this works correctly if I leave out_dir and root_dir unset in the macro.
I'm also open to using some other source of info to get the current package path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just drop the use of rootDir here? Normally outDir || '.' would be create if we're talking about the tsconfig outDir...

Copy link
Contributor Author

@malt3 malt3 Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since that would create outputs directly in Bazel's BINDIR. Actions are only allowed to write below their own package dir.
I tested the behavior of typescript closely, and . would only work for a BUILD file in the topmost directory.
I can also add a test for this or demonstrate the behavior with an example if needed.
As stated above, maybe we can use some other method to pass the current package as a default, without relying on rootDir?
Note that the worker doesn't cd into the rootDir.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this should probably be outDir || BUILD-dir? I still think rootDir is wrong here, correct me if this is different then the tsconfig rootDir though...

If you can add some tests that would be ideal 👍

Copy link
Contributor Author

@malt3 malt3 Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, outDir || BUILD-dir would be ideal. It's just that the long-lived worker process can only infer the BUILD directory from the work request. Right now this doesn't contain the BUILD directory. I just chose rootDir since it happens to match BUILD-dir by default.

I can add a test, but more importantly: should we add the current package (aka BUILD-dir) to the work request explicitly instead of misusing rootDir?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I don't know this worker code well and I'm going off assumptions here. So maybe this SYNTHETIC_OUTDIR logic is different then what I'm assuming.

Can we simply avoid this SYNTHETIC_OUTDIR thing when outDir is not configured?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rules_ts/ts/private/ts_project_worker.js

Lines 547 to 552 in 0d1dadb

const bin = process.cwd(); // <execroot>/bazel-bin/<cfg>/bin
const execroot = path.resolve(bin, '..', '..', '..'); // execroot
const tsconfig = path.relative(execroot, path.resolve(bin, cmd.options.project)); // bazel-bin/<cfg>/bin/<pkg>/<options.project>
const cfg = path.relative(execroot, bin) // /bazel-bin/<cfg>/bin
const executingFilePath = path.relative(execroot, require.resolve("typescript")); // /bazel-bin/<opt-cfg>/bin/node_modules/tsc/...
const executingDirectoryPath = path.dirname(executingFilePath);
one of these should give you the package relative path.

if (host.optionsToExtend.declarationDir) {
host.optionsToExtend.declarationDir = `${host.optionsToExtend.declarationDir}/${SYNTHETIC_OUTDIR}`
}
Expand Down