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

Output compilerOption Diagnostics #191

Closed

Conversation

samcooke98
Copy link

Rollup Plugin Name: Typescript

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Typescript Rollup didn't output compiler options diagnostics. The most obvious case where this fails is when the supplied tsconfig leaves composite: true. (as declaration is ignored by rollup plugin)

Previously, the plugin would fail silently, and print no detail about why it failed.

./src/index.ts → ./dist/index.js...
[!] (typescript plugin) Error: Couldn't compile 
src/index.ts
Error: Couldn't compile src/index.ts
    at Object.transform (/node_modules/@rollup/plugin-typescript/dist/index.js:466:23)
    at /node_modules/rollup/dist/rollup.js:16611:25

By adding the services.getCompilerOptionsDiagnostics() call the plugin will emit reasons for the failure.

);

t.true(
err.message.includes('Composite projects may not disable declaration emit'),
Copy link
Member

Choose a reason for hiding this comment

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

Should we override this flag too?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add composite to our list of overridden compiler options if it causes an error.

Copy link
Author

@samcooke98 samcooke98 Feb 2, 2020

Choose a reason for hiding this comment

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

I agree - I think this is best done in a separate PR however(although if you want it done in this one too, I'm happy to do it)

It seems ideal to output compilerOptions errors, regardless of the overridden compiler options - Future typescript versions may introduce new compilerOption errors.

(And thinking about it, when I do this in a separate PR, it'll break this test)

Copy link
Member

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@shellscape
Copy link
Collaborator

@samcooke98 apologies that this one sat so long. Any chance you'd be willing to address the conflicts and failing CI?

@samcooke98
Copy link
Author

@samcooke98 apologies that this one sat so long. Any chance you'd be willing to address the conflicts and failing CI?

No worries, it's my bad too, it slipped off my radar. I'll try to get to it this weekend (ie within the next 3 days)

@shellscape
Copy link
Collaborator

@samcooke98 we had some CI shenanigans going that were fixed up today. Please do a merge from master and push and it should start passing.

@samcooke98
Copy link
Author

Just tested this with@rollup/[email protected] and couldn't reproduce - seems fixed?

Will close now (Sorry I forgot about this PR)

@samcooke98 samcooke98 closed this Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants