Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

add support for tsconfig with "extends" #153

Merged
merged 1 commit into from
Nov 29, 2019

Conversation

tolu
Copy link
Contributor

@tolu tolu commented Nov 29, 2019

I know you're in the process of moving the repo, and I'd be glad to submit this PR to the new repo (when it's up), but I'd though I just give it a try anyway.

Changes

When the original tsconfig file contains a "extends" property, use typescript.parseJsonConfigFileContent to parse the entire inheritance chain, providing the already parsed config as existingOptions.

Tests

Added test where the base config is the one that has the "jsx": "react" required to make the test pass (copied the tsconfig-jsx test case)

@shellscape
Copy link
Contributor

Great timing on this one. We're getting ready to archive this repo for the move of the plugin to rollup/plugins and will do our best to get this merged.

Copy link
Contributor

@shellscape shellscape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but a few comments on conveying errors to users that need to be addressed.

null :
typescript.parseJsonConfigFileContent(tsConfig, typescript.sys, process.cwd(), parsed.options);
if (extendedConfig && extendedConfig.errors.length) {
extendedConfig.errors.forEach( error => console.error( `rollup-plugin-typescript: ${ error.messageText }` ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we generally want to shy away from our own console output here. instead, use this.warn

Copy link
Member

@NotWoods NotWoods Nov 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This plugin is currently using console.error rather than this.warn, but we're switching it for the migration. You can choose to switch it now or I can switch it later when migrating 🙂

if (extendedConfig && extendedConfig.errors.length) {
extendedConfig.errors.forEach( error => console.error( `rollup-plugin-typescript: ${ error.messageText }` ) );

throw new Error( `rollup-plugin-typescript: Couldn't process compiler options` );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and use this.error here, as it will let Rollup handle the output and abort the bundling process properly.

@shellscape shellscape mentioned this pull request Nov 29, 2019
@tolu
Copy link
Contributor Author

tolu commented Nov 29, 2019

Thanks for the feedback! I'm on it! 😄

@tolu
Copy link
Contributor Author

tolu commented Nov 29, 2019

Question.

As far as I understand the docs and the plugin structure, in the body of the plugin, i.e. anything before the hooks, I don't have access to this as a plugin context. this is undefined, and so if we don't move the entire typescript config read into a hook, the whole thing crashes (at least in my tests).

Can you help me understand how to get that going?
Move reading of compilerOptions to buildStart hook?

@shellscape
Copy link
Contributor

@tolu that'd be the logical spot. I also see that there is some console.error use in transform. I'm going to dismiss my requested changes on this as I didn't realize that'd be a larger refactor, which we can tackle after @NotWoods and I move the plugin to rollup/plugins.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants