Skip to content
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

tsdx ignores tsconfig.json "extends" keyword #484

Closed
justingrant opened this issue Feb 2, 2020 · 20 comments · Fixed by #489
Closed

tsdx ignores tsconfig.json "extends" keyword #484

justingrant opened this issue Feb 2, 2020 · 20 comments · Fixed by #489
Labels
help wanted Extra attention is needed kind: bug Something isn't working

Comments

@justingrant
Copy link

Current Behavior

There's an extends feature in tsconfig which allows configuration inheritance across multiple tsconfig files. This however isn't recognized in tsdx, which parses tsconfig here:

let tsconfigJSON;
try {
tsconfigJSON = await fs.readJSON(paths.tsconfigJson);
} catch (e) {}

The JSON object is used as-is, instead of drilling up to resolve parent config.

Expected behavior

tsdx should process extends just like tsc does.

Suggested solution(s)

In a quick look through TypeScript's source code I couldn't find the place where extends is handled, but it's possible that TypeScript's exported tsconfig parsing functions might handle extends for us automatically. See https://github.com/microsoft/TypeScript/blob/760393f893358a462b00eef7e8fb92342b0f71ad/lib/typescript.d.ts#L3639-L3647 what what TS offers in its API. The implementation is here: https://github.com/Microsoft/TypeScript/blob/6487d1ffe0d903b1e55a187ddeb973fe0d445a2f/src/compiler/parser.ts#L717

Additional context

This problem is similar to #483 in that both this issue and that one are caused by parsing tsconfig differently than tsc does.

Your environment

Software Version(s)
TSDX [email protected]
TypeScript [email protected]
Browser n/a (this is a build issue)
npm/Yarn [email protected]
Node v12.13.1
Operating System MacOS Catalina 10.15.2
@swyxio
Copy link
Collaborator

swyxio commented Feb 3, 2020

great qtn. we'll probably want to handle this.

@swyxio swyxio added good first issue Good for newcomers help wanted Extra attention is needed labels Feb 3, 2020
@justingrant

This comment has been minimized.

@swyxio

This comment has been minimized.

@ncphillips
Copy link
Contributor

👍 this would be great for typescript monorepos. Some of the tinacms packages use tsdx for builds. Ideally most of the config would be in the root tsconfig.json with the occasional special case in the package's.

@justingrant
Copy link
Author

justingrant commented Feb 4, 2020

Echoing @ncphillips, I originally ended up here at tsdx because my project depends on the https://github.com/reach/reach-ui monorepo that uses tsdx and uses extends in tsconfig.json. I wanted to help reach-ui get its declaration maps fixed in its monorepo. But without extends support any fix will involve making changes to ~20 different packages' tsconfig.json files, which is not ideal.

Luckily the only rollup settings pulled direct from JSON by tsdx are esModules (which doesn't actually use tsconfig because of #469) and useTsconfigDeclarationDir (once #468 is merged). In other words, currently this issue has no impact... but it will have impact once those other two issues are resolved.

@swyxio
Copy link
Collaborator

swyxio commented Feb 4, 2020

anyone want to tackle this?

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 4, 2020

anyone want to tackle this?

Oh, I already did hah. @justingrant said he didn't have time for this in #483 (comment) so I made the solution myself (see above reference). Still need to add a test for the extends case, hence the draft, but will do that in a day or few as I have to focus on some other things

@swyxio
Copy link
Collaborator

swyxio commented Feb 4, 2020

ok. @agilgur5 we're piling up quite a few PR's for jared to review, lets slow down so that we dont swamp him with too much stuff.

@jaredpalmer
Copy link
Owner

Hrmmm. I thought this worked? Maybe a false positive. Let’s fix

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 4, 2020

Hrmmm. I thought this worked? Maybe a false positive. Let’s fix

@jaredpalmer see #484 (comment) . #483 also has a more detailed description. This is specifically for TSDX-specific options like esModuleInterop. It is already handled by rollup-plugin-typescript2 for the most part, but some tsconfig options affect TSDX too.
Also already made a PR yesterday as I mentioned and referenced above (#489)

@agilgur5
Copy link
Collaborator

agilgur5 commented Feb 4, 2020

ok. @agilgur5 we're piling up quite a few PR's for jared to review, lets slow down so that we dont swamp him with too much stuff.

@sw-yx think that might be the first time I've been asked to contribute less 🤔 🤔 🤔

@swyxio
Copy link
Collaborator

swyxio commented Feb 4, 2020

i mean i just want to keep it manageable for jared. its not my name on the repo heheh

@ncphillips
Copy link
Contributor

On the mention of contributing less, I'm just going to leave this reference to theory of constraints here 😄

@jessekrubin

This comment has been minimized.

@agilgur5
Copy link
Collaborator

@jessekrubin are you sure you were facing this issue? As written above a few times, currently this only affects the esModuleInterop setting, and that actually has a bug (#469), so this issue currently has 0 impact on anything (it only has future impact like #468 )

@jessekrubin

This comment has been minimized.

@agilgur5

This comment has been minimized.

@jessekrubin

This comment has been minimized.

@agilgur5

This comment has been minimized.

@agilgur5
Copy link
Collaborator

Repeating here from #483 :

Merged in #489 as I'm now a collaborator. Turns out that ts.parseJsonConfigFileContent is needed to parse extends, but ts.readConfigFile is enough for comments and commas. These are not remotely well documented, so I had to search around other repos to figure out how to do this simply and properly. Can read the PR comments for what I searched through

@agilgur5 agilgur5 removed the good first issue Good for newcomers label Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed kind: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants