Skip to content

Throw on rollup build warnings - #2016

Merged
lukastaegert merged 2 commits into
masterfrom
throw-on-warnings
Mar 5, 2018
Merged

Throw on rollup build warnings#2016
lukastaegert merged 2 commits into
masterfrom
throw-on-warnings

Conversation

@lukastaegert

Copy link
Copy Markdown
Member

This PR will throw if building rollup itself displays a warning.

In the past, we often had issues with PRs that accidentally introduced external dependencies (usually node builtins) into the browser build. The main reason was that warnings in the build process were easily overlooked. This is now changed by throwing an error in these situations that will prevent a successful build until all warnings are fixed.

Errors will be thrown for any kind of warnings, including circular dependencies. To make this possible, some definitions had to be moved around to resolve the existing circles. Should there ever be a situation where circular dependencies cannot be avoided, I would suggest to add an exception to the onwarn handler. However I would find it preferable to avoid them altogether as they can always be an indication of architectural problems.

@guybedford

Copy link
Copy Markdown
Contributor

I'm strongly against this actually. I think Rollup already can be a little fickle to get a build working with NodeJS workflows, and the solution to that is to make NodeJS workflows easier not harder.

I'd like to work towards a place where we have a reliable NodeJS plugin that handles all builtins transparently and does everything one would expect as in Browserify. My current goal is to provide a new rollup-node project that provides this within the next two months.

Also note that some circular warnings are unavoidable when building eg Node streams which are third-party circular references by design that users can't necessarily control.

If something like that could remove these problems, what remaining rationale would there be for having all warnings throw?

@lukastaegert

Copy link
Copy Markdown
Member Author

I think you totally misunderstand this PR. This is just about our internal workflow. This will change NOTHING for users of rollup. The core change is basically a change to our internal rollup.config.js. Maybe just take a look, it is the second commit.

@lukastaegert

Copy link
Copy Markdown
Member Author

Also, I have no intention to break builds for other users. That would be horrible indeed. This is just for us.

@lukastaegert

Copy link
Copy Markdown
Member Author

This PR will throw if building rollup itself displays a warning

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh I see, yes I completely misunderstood this was just for our build!

Interface refactorings seem nice too.

@lukastaegert

Copy link
Copy Markdown
Member Author

Thanks 😉👍

@lukastaegert

Copy link
Copy Markdown
Member Author

Also I really like that idea of a rollup-node plugin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants