Add system argument to readConfigFile #4497
Conversation
Which allows the caller to specify the `System` used to read the file.
| @@ -374,10 +374,10 @@ namespace ts { | |||
| * Read tsconfig.json file | |||
| * @param fileName The path to the config file | |||
| */ | |||
| export function readConfigFile(fileName: string): { config?: any; error?: Diagnostic } { | |||
| export function readConfigFile(fileName: string, system: System = sys): { config?: any; error?: Diagnostic } { | |||
There was a problem hiding this comment.
Given that it's only used once in the compiler, I'd make this non-optional.
There was a problem hiding this comment.
I agree completely (and have followed up by doing so). But we should acknowledge this is a breaking change to a public API. (exported in the ts namespace without a /* @internal */ annotation)
There was a problem hiding this comment.
Keep it optional so this is not a breaking change.
weswigham
changed the title
| @@ -374,10 +374,10 @@ namespace ts { | |||
| * Read tsconfig.json file | |||
| * @param fileName The path to the config file | |||
| */ | |||
| export function readConfigFile(fileName: string): { config?: any; error?: Diagnostic } { | |||
| export function readConfigFile(fileName: string, system: System): { config?: any; error?: Diagnostic } { | |||
There was a problem hiding this comment.
I would use readFile: (path: string) => string instead of full-blown sys as an optional argument. For consumers that does not have sys it will be simpler then supplying implementation of System where everything but readFile throws Error("Unsupported") or something like that.
There was a problem hiding this comment.
Fair. It is really all it cares about.
|
why is this needed? |
|
I'd say this is a cleanup work that will our API more transparent and easy to use on environments where |
|
Less needed, more for consistency. Up until 8 days ago the language service server called |
|
thanks for the explanation. 👍 |
Add system argument to readConfigFile
|
Why it was merged? Currently it is a breaking change because new argument is non optional? |
|
You're right, and nobody picked up on it because continuing commenting on outdated diffs is odd. >.> Would we like to fix it? |
|
I would say yes, there is no compelling reason for this to be a breaking change |
Which allows the caller to specify the
Systemused to read the file.