Make rollup warnings build errors. #6850
Conversation
This will ensure that all messages that are emitted to the console by rollup trigger a build failure and therefore prevent landing any changes to Ember Data that would introduce such a warning.
rwjblue
force-pushed
the
do-not-tolerate-warnings
branch
from
df6cef0 to
eb60be5
Compare
There was a problem hiding this comment.
we already throw an error at build unless this is in a consuming app instead of our own, so I'm not sure what the advantage here is.
Edit: The intent here is to throw for any unhandled warning and not just the ones we know are due to us and how to handle.
I'm not sure how I feel about the potential work interruption when we encounter a rollup warning we haven't learned how to handle yet. The project halting for something that isn't required to be resolved immediately seems bad.
Since these are just warnings and not build errors, might be better to instead of throwing for all use an ENV flag to conditionally print the unhandled warnings when not locally developing and suppress them the rest of the time for consumers.
| if (message.code === 'CIRCULAR_DEPENDENCY') { | ||
| return; | ||
| } else if (message.code === 'NON_EXISTENT_EXPORT') { | ||
| } else if (message.code === 'NON_EXISTENT_EXPORT' && message.message.includes('ts-interfaces')) { |
There was a problem hiding this comment.
the removed slashes were there intentionally although I do see how it might look like a borked regex ;)
It's unlikely for this shorter string to match anything the longer string didn't match but the slashes were insurance we were finding a warning that included this string as part of a file path.
On the code style change I'm mixed. Folks rushed into using contains and includes and startsWith in our library and test code and I recently had to revert all of that to get us back to IE11 support. I like using new things but I dislike mixing styles. Overall though this is clearly a build file so its a bit more clear that it works here.
There was a problem hiding this comment.
Folks rushed into using contains and includes and startsWith in our library and test code and I recently had to revert all of that to get us back to IE11 support. I like using new things but I dislike mixing styles.
Ya, but this is Node code and has no requirement for IE11 support.
| return; | ||
| } | ||
| } else if (message.code === 'UNRESOLVED_IMPORT') { | ||
| if (!this.isDevelopingAddon()) { |
There was a problem hiding this comment.
we shouldn't remove this check as rollup, babel, rollup plugins, broccoli, and broccoli-rollup may all be a different version in a consuming app than what we test and ship with. It's better to print an unnecessary warning than to break the build for those folks.
| const chalk = require('chalk'); | ||
| // make warning actionable | ||
| // eslint-disable-next-line no-console | ||
| console.log( |
There was a problem hiding this comment.
this warning (which is nicely actionable) would not pertain to any errors that don't match the removed message.code === 'UNRESOLVED_IMPORT'. We should bring back that condition.
|
These have been build errors on master for some time, closing as unnecessary |
This will ensure that all messages that are emitted to the console by rollup trigger a build failure and therefore prevent landing any changes to Ember Data that would introduce such a warning.
Follow up to #6809 (comment)