-
Notifications
You must be signed in to change notification settings - Fork 508
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.
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
(fix): parse tsconfig extends, trailing commas, and comments #489
Conversation
ab0e3f9
to
e65e317
Compare
e65e317
to
b62312c
Compare
Fix conflicts |
d670c4b
to
21860c1
Compare
Done. |
21860c1
to
eede53d
Compare
Welp, so since I merged #468 , I switched to use that fixture, which actually does something different than the default, i.e. sets Turns out Seems like the same issue encountered in kulshekhar/ts-jest#245 (comment) , the solution to which is non-trivial... welppppp. investigating alternatives right now |
eede53d
to
7758560
Compare
Ok, some links I went through to figure this out:
So ended up using that last one |
- use ts.readConfigFile to properly parse a commas and comments from tsconfig instead of just trying to readJSON - use ts.parseJsonConfigFileContent to properly parse `extends` and get recursive compilerOptions - add tests for all of the above use cases - add them to build-withTsconfig which changes declarationDir; should only work if TSDX properly parses the tsconfig NOTE: this is only necessary for internal, TSDX-specific parsing of tsconfig.json. rollup-plugin-typescript2 already handles these options, but internal options like esModuleInterop etc are also used & parsed independently of rpts2
7758560
to
f2ae526
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I reduced the necessary changes for extends
to just a few lines, so there wasn't much to review.
I'm still a bit skeptical of the './'
for ts.parseJsonConfigFileContent
's third argument, basePath
, but this passed all of my manual tests too. I'm most concerned about usage with custom tsconfig path #436 especially since we don't have tests for that. Will have to add some soon.
…lmer#489) - use ts.readConfigFile to properly parse a commas and comments from tsconfig instead of just trying to readJSON - use ts.parseJsonConfigFileContent to properly parse `extends` and get recursive compilerOptions - add tests for all of the above use cases - add them to build-withTsconfig which changes declarationDir; should only work if TSDX properly parses the tsconfig NOTE: this is only necessary for internal, TSDX-specific parsing of tsconfig.json. rollup-plugin-typescript2 already handles these options, but internal options like esModuleInterop etc are also used & parsed independently of rpts2
use ts.readConfigFile to properly parse a commas and comments from
tsconfig instead of just trying to readJSON
use ts.parseJsonConfigFileContent to properly parse
extends
andget recursive compilerOptions
add tests for all of the above use cases
should only work if TSDX properly parses the tsconfig
NOTE: this is only necessary for internal, TSDX-specific parsing of
tsconfig.json. rollup-plugin-typescript2 already handles these options,
but internal options like esModuleInterop etc are also used & parsed
independently of rpts2
Fixes #483
with its own suggestion, eventually getting to facebook/create-react-app#7248 as the solutionEDIT: that only solved commas and comments,extends
needed more work, see below comments.Also fixes #484 as it's the same parser that does both. Still need to add tests for this case, thinking of just having all test fixtures extend a base fixture and then override it?
This is built on top of #436 , please merge that first.