Leave files from node_modules out when calculating getCommonSourceDirectory
#11993
Conversation
ghost
force-pushed
the
common_source_directory
branch
from
0faeba2 to
e93c776
Compare
ghost
assigned
ghost
force-pushed
the
common_source_directory
branch
from
e93c776 to
414589f
Compare
|
I think @vladima should take a look too. |
| if (typeof commonSourceDirectory === "undefined") { | ||
| if (options.rootDir && checkSourceFilesBelongToPath(files, options.rootDir)) { | ||
| if (commonSourceDirectory === undefined) { | ||
| const emittedFiles = filterSourceFilesInDirectory(files, isSourceFileFromExternalLibrary); |
There was a problem hiding this comment.
This is ignoring .d.ts files in the own compilation as well ? (not from external library)
There was a problem hiding this comment.
Yes. For the --outDir case that makes sense since we are just emitting .ts files. Can you provide another case where we would want the .d.ts files included?
There was a problem hiding this comment.
But right now we didnt ignore .d.ts files from our own program to compute the common source directory? So this is intended change?
There was a problem hiding this comment.
I've added a .d.ts file to the test.
There was a problem hiding this comment.
you need a new test case:
//@filename: app/src/index.ts
//@filename: app/lib/bar.d.ts
//@filename: app/tsconfig.json
To make sure common source directory is still app
There was a problem hiding this comment.
OK, I added a test commonSourceDirectory_dts.ts. This appears to already be the behavior of the current compiler -- the referenced file does not become part of the output.
| @@ -430,13 +430,14 @@ namespace ts { | |||
| return program; | |||
|
|
|||
| function getCommonSourceDirectory() { | |||
| if (typeof commonSourceDirectory === "undefined") { | |||
| if (options.rootDir && checkSourceFilesBelongToPath(files, options.rootDir)) { | |||
| if (commonSourceDirectory === undefined) { | |||
There was a problem hiding this comment.
This is also used by maproot and sourceroot.. So not doing this for bundle emit has effect on how the paths would be represented... Because --mapRoot --sourceRoot --outFile is a valid option
There was a problem hiding this comment.
when the files exist in node_modules and src folder and --sourceroot is specified?
There was a problem hiding this comment.
If --sourceRoot is specified, then the "sourceRoot" property in the source map is whatever you specified.
There was a problem hiding this comment.
yes but the paths are relative to common source directory and with this change the commonsourcedirectory would be different from what it used to be if say eg your sources are in ./src/ and ./node_modules/ which means the sources will be just relative to ./src/ instead or ./
There was a problem hiding this comment.
I think this is the section @sheetalkamat is referring to: https://github.com/Microsoft/TypeScript/blob/4a906143c62fcbfe113bf3f1cc607c488df47400/src/compiler/sourcemap.ts#L171-L180
There was a problem hiding this comment.
I wrote a test case. @sheetalkamat could you look at the baselines and tell me if they're what you would expect?
There was a problem hiding this comment.
I think test case is ok, and realized that scenario I was talking about sourceroot and maproot works because --out is only allowed with amd or system and hence there is no way to get bundled emit that includes modules from node_modules folder.
ghost
force-pushed
the
common_source_directory
branch
from
414589f to
2eca0af
Compare
ghost
force-pushed
the
common_source_directory
branch
from
9006572 to
0b71df5
Compare
|
|
||
|
|
||
| //// [/app/bin/index.js] | ||
| "use strict"; | ||
| /// <reference path="../types/bar.d.ts"/> |
There was a problem hiding this comment.
this is my point... without this change it would have been bin/app/index.js -- so is this really what we want.. and this would be breaking change.. @mhegazy to take a look and confirm this is indeed expected change...
| if (typeof commonSourceDirectory === "undefined") { | ||
| if (options.rootDir && checkSourceFilesBelongToPath(files, options.rootDir)) { | ||
| if (commonSourceDirectory === undefined) { | ||
| const emittedFiles = filterSourceFilesInDirectory(files, isSourceFileFromExternalLibrary); |
There was a problem hiding this comment.
you need a new test case:
//@filename: app/src/index.ts
//@filename: app/lib/bar.d.ts
//@filename: app/tsconfig.json
To make sure common source directory is still app
| @@ -430,13 +430,14 @@ namespace ts { | |||
| return program; | |||
|
|
|||
| function getCommonSourceDirectory() { | |||
| if (typeof commonSourceDirectory === "undefined") { | |||
| if (options.rootDir && checkSourceFilesBelongToPath(files, options.rootDir)) { | |||
| if (commonSourceDirectory === undefined) { | |||
There was a problem hiding this comment.
I think test case is ok, and realized that scenario I was talking about sourceroot and maproot works because --out is only allowed with amd or system and hence there is no way to get bundled emit that includes modules from node_modules folder.
ghost
force-pushed
the
common_source_directory
branch
from
d4786ad to
0422a75
Compare
Fixes #11977