Use common source directory for calculating concatenated module paths

[weswigham]

Rather than the current directory.

[mhegazy]

👍

[mhegazy]

can you give this a try on #5471 and see if it failed tests @sheetalkamat is seeing?

[sheetalkamat]

@weswigham you would need to make sure when running project test cases (with #5471) that there are no *.dts.errors.txt is generated

[weswigham]

Additionally, I needed to change program to always compute a common source directory - that we sometimes never bothered to compute it caused certain configurations of arguments to fail. Given that so many configurations require it at this point, it's better to just always have a directory ready. Plus, it was really weird that host.getCommonSourceDirectory()'s undefined-ness was reliant on compiler flags. Any external implementer of an emit host would not be so unpredictable, I think.

[weswigham]

I've been able to merge this PR with #5471 locally and the tests all pass except getEmitOutputSingleFile2 which has a baseline change where the module name "tests/cases/fourslash/inputFile3" is instead "inputFile3", which is in-line with the change of using common source directory here.

So it looks good now. 👍

[sheetalkamat]

@weswigham I don't think it is good idea to calculate common source directory irrespective of options. We should be calculating it only for the options that use it. Because first it is unnecessary and second it should be reporting error on not finding common source directory and it would be wrong to do that if we aren't going to use it.

[weswigham]

@sheetalkamat: what about if I defer the error reporting until getCommonSourceDirectory() is actually invoked? This way the error is reported when its relevant and you never have to worry about if a common source directory has been generated or not.

[weswigham]

@sheetalkamat on that - I've actually already deferred it to error at the same time it would have before in #5590. Taken together they seem fine.

[sheetalkamat]

@weswigham Question is should it be error if there is no common source directory when --outFile is specified? I guess not. Can you add a test case with such scenario, so we can see how the output will look especially in case of combining external modules.

I still don't like the idea that we do work related to finding common source directory when its not needed. Special consideration to project with lots of sources.

[weswigham]

@sheetalkamat: Maybe it would simply be best to actually put the generation of a common source directory within the get common source directory function (Including logic to cache it), then? (And calling it in verifyCompilerOptions if we need to check for error scenarios) This way there's no cognitive overhead of needing to know which compiler options force generation of one and no runtime overhead of generating one when its not used...

[weswigham]

Also, I will add a test for that - I'm not sure what we do for that today.

[weswigham]

@sheetalkamat do you wanna look at this again?

[sheetalkamat]

👍

[sheetalkamat]

A question though... should --rootDir be used if specified as common source directory?

[weswigham]

It is used - it gets set as the value of commonSourceDirectory in verifyCompilerOptions if present and all source files are within it (and the verify compiler options check has decided that it needs to pregenerate a common source directory).

[sheetalkamat]

@weswigham but that happens only if --outDir, --sourcRoot or --mapRoot
Here!

[weswigham]

@sheetalkamat correct. I guess it would be possible to use --rootDir without any of those and still be in a situation where you'd like to get a common source directory. (This totally needs a test) I suppose I should pull that same check for rootDir into the default value function. This could actually simplify the verify code a bit, too. I assume the errors we emit with rootDir are valid in all cases we'd want to find a common source directory, so I can continue to use checkSourceFilesBelongToPath and its diagnostics within it.