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.

Sign up for GitHub

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

Make rollup warnings build errors. #6850

Closed
wants to merge 1 commit into from
Closed

Make rollup warnings build errors. #6850

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 19 additions & 26 deletions packages/private-build-infra/src/addon-build-config-for-data-package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,35 +45,28 @@ function addonBuildConfigForDataPackage(PackageName) {
}
},

_suppressUneededRollupWarnings(message, next) {
_handleRollupWarning(message) {
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')) {
Copy link
Contributor

@runspired runspired Dec 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// ignore ts-interface imports
if (message.message.indexOf(`/ts-interfaces/`) !== -1) {
return;
}
} else if (message.code === 'UNRESOLVED_IMPORT') {
if (!this.isDevelopingAddon()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// don't print these for consumers
return;
} else {
const chalk = require('chalk');
// make warning actionable
// eslint-disable-next-line no-console
console.log(
chalk.yellow(
`\n\n⚠️ Add ${chalk.white(
message.source
)} to the array returned by externalDependenciesForPrivateModule in index.js of ${chalk.white(
this.name
)}\n\n`
)
);
throw message.message;
}
return;
} else {
const chalk = require('chalk');
// make warning actionable
// eslint-disable-next-line no-console
console.log(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

chalk.yellow(
`\n\n⚠️ Add ${chalk.white(
message.source
)} to the array returned by externalDependenciesForPrivateModule in index.js of ${chalk.white(
this.name
)}\n\n`
)
);

throw message.message;
}
next(message);
},

getOutputDirForVersion() {
Expand Down Expand Up @@ -141,7 +134,7 @@ function addonBuildConfigForDataPackage(PackageName) {
packageName: PackageName,
babelCompiler: babel,
babelOptions: this.options.babel,
onWarn: this._suppressUneededRollupWarnings.bind(this),
onWarn: this._handleRollupWarning.bind(this),
externalDependencies: this.externalDependenciesForPrivateModule(),
destDir: this.getOutputDirForVersion(),
});
Expand Down