GeezerVison

Hi Jonathan Protzenko (defunct account, use @protz now) (@msprotz), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Jonathan Protzenko). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

This is a preliminary pull request to discuss the proposed change. (There's no test at the moment.) I'd be happy to know whether the change looks good, or if you'd rather have me modify something.

Thanks,

Jonathan

Thanks for the PR.

+1 for the flag, i think it is a common pain point and we need to fix it. A few comments though,

• I do not think it should be "projectRoot" i do not think the word project is well defined in this context. I think it is more about CommonSourceDirectory calculation, and the name should reflect that. May be "commonSourceRoot" (it should have been soruceRoot, but unfortunately we used that to mean something slightly different).
• We would need to check that it is really a commonDirectory, sometimes a common path is not possible (e.g. sources on different directories) and we emit files to locations relative to the common path, and if that is not correct you could overwrite files silently, and that is bad. verifyCompilerOptions in program.ts is a good place for that checking, and it already has some of that logic.
• Once you have figured out the common path, you just want the normal soucemap emit to happen, without jamming the projectRoot value in there. that is what sourceMap is for.
• you also need tests :), look at project tests under tests\cases\project (and their contents under tests\cases\projects) for examples.

Thanks for the review!

1. I agree about changing the name of the option, commonSourceRoot seems fine.

2. I don't understand your remark about overwriting files silently. The option I introduce has no effect whatsoever on code generation, it only affects the sources field of the sourceMap. Here's an example.

# without the patch
protz@Joprotze-Z420:/cygdrive/d/t $ tsc --sourceRoot "http://localhost/editor/local" --sourceMap --module commonjs a/a.ts
protz@Joprotze-Z420:/cygdrive/d/t $ cat a/a.js.map 
{"version":3,"file":"a.js","sourceRoot":"http://localhost/editor/local/","sources":["a.ts"],"names":["a"],"mappings":"AAAA,SAAgB,CAAC;AAAKA,CAACA;AAAP,SAAC,GAAD,CAAO,CAAA"}

# with the patch
protz@Joprotze-Z420:/cygdrive/d/t $ node ../TypeScript/built/local/tsc.js --projectRoot . --sourceRoot "http://localhost/editor/local/" --sourceMap --module commonjs a/a.ts && cat a/a.js.map
{"version":3,"file":"a.js","sourceRoot":"http://localhost/editor/local/","sources":["a/a.ts"],"names":[],"mappings":"AAAA,SAAgB,CAAC;AAAD,SAAC,GAAD,CAAO,CAAA"}

Which files get generated, and where they get generated, remains unchanged.

Furthermore, there is no way the code can figure out by itself that the common source root is ., not ./a... so I don't think I can improve the common source directory calculation, because in the example above, we just can't compute it.

Sorry for the delay. yes that is one application of a commonRoot, other ones are related to --outDir use, where you want to mimic the input directory structure in the output (this is where my overwrite comment fits). I thnink plugging this in the common path calculation would work for outDir as well as your scenario, is that accurate?

Jonathan Protzenko (defunct account, use @protz now) (@msprotz) can you refresh and resubmit?

Shouldn't commonSourceRoot be driven by the location of tsconfig.json ?

it think this is fair. currently it is the longest common path of all input files.. so

a
| __ c 
       | __ b.ts
       | __ c.ts

so the common root is a\c

I would expect:

a
| __ tsconfig.json
| __ c 
       | __ b.ts
       | __ c.ts

the common root to be a.

Consider tsconfig.json with an outDir ./b. I would expect:

a
| __ tsconfig.json
| __ c 
       | __ b.ts
       | __ c.ts
| __ b
       | __ c 
              | __ b.js
              | __ c.js

i.e copy the entire structure starting from tsconfig.json into the directory b.

i think this makes sense.

I would say we need a new commandline argument, to allow you to change that if you are not using a tsconfig.json or you want to modified. i am thinking of --commonSourceRoot.

The suggestions above definitely make sense, both for the output file hierarchy and the source maps. The trick about using tsconfig.json or, if no such file is present, an option, seems good to me.

(--commonSourceRoot is fine, I also thought of it in #2034 (comment)).

I just need to update the patch so that the option is also used when computing the output file names. I'll try to find some time to do that soon.

I've requested something like this in the past, I called it baseDir : #287 was rejected. I did end up agreeing with Ryan Cavanaugh (@RyanCavanaugh)'s choice of not adding another tsc flag that can be taken care elsewhere (even though it wasn't at that time).

I also like it to be implict now that we have tsconfig.json. Don't want to end up with:

a
| __ tsconfig.json
| __ c 
       | __ b.ts
       | __ c.ts
       | __ d 
              | __ b.ts
              | __ c.ts

where tsconfig.json has a completely irrational commonSourceRoot like ./c/d.

tsconfig.json is the source root.

Basarat Ali Syed (@basarat), as i started working on this I realized that the implicit tsconfig.json path will not work, as the rootDir/baseDir has to encompass all input files, but tsconfig.json does not have this restriction. "files" property can reference files outside the directory, and /// references as well as imports can pull in files from outside the directory. we do not want to make these an error, specially that you only care about this if you have outDir. you can always specify a rootDir in your tsconfig to always be "." to work around this.

Okay ❤️

"files" property can reference files outside the directory

Worth mentioning: If someone does that then they are going to have a really hard time if they open a file that doesn't have tsconfig.json up the directory tree.