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
errors reported with onwarn #2582
Comments
I understand your issue. The reason why accessing (and possibly calling) a non-exported variable on a namespace object is not an error is that this is actually valid JavaScript and might actually be intended in certain situations. If you want an error to be thrown, you can check the provided warning in |
However, the generated code is hardly valid: (function () {
'use strict';
undefined("abc");
}()); For a warning case, the original variables should be preserved instead of substituting them with undefined. |
It valid in the sense that it is parseable JavaScript and only throws a runtime exception. The behaviour is also 100% consistent with what would happen if you run this code directly in an ESM environment such as a modern browser. If instead of calling the missing export you would just check for its presence, this behaviour would make sense and have the correct result. We are of course aware that this is rarely what you what which is why you get the warning. |
The point is, there is no "original variable" as internally, the namespace does not exist. According to the spec, a missing export when accessed via a namespace object is equivalent to undefined. |
I understand your point. Assuming someone runs rollup with --silent flag, such warning/error will never be seen. As the process exits with 0 and this will be a surprising runtime error. In a more practical context, one could get this as a result of not using CommonJS plugin to rollup a script with a CommonJS dependency. The original code works, but the rolled up version doesn't and it will take time to figure out the root cause. As you have said such behavior is almost certainly to be undesired, why is still a warning instead of an error. As a comparison closure compiler will fail a build loudly in this kind of cases, unless certain flags are passed or source code is annotated to supress the error. IMHO, rollup could introduce a strict mode to report all these errors as error and let the warning be just warnings. |
As far as I can tell, even throwing inside |
Thinking about this issue more (there have been similar discussions about circular dependencies, albeit here people were looking for a way to get rid of some warnings), maybe an option to filter warnings would be a good way forward. Here is an initial suggestion: Elevate some warnings to errors and silence others export default ({
input: '...',
warnings: {
// valid values could be
// 'error': throw an error instead of a warning
// true: keep this as a warning (the default, could also be 'warning')
// false: silence this warning
MISSING_EXPORT: 'error',
CIRCULAR_DEPENDENCY: false
},
output: { /* ... */ }
}); Elevate ALL warnings to errors with some exceptions export default ({
input: '...',
warnings: {
all: 'error',
MISSING_EXPORT: true,
CIRCULAR_DEPENDENCY: false
},
output: { /* ... */ }
}); Elevate all warnings to errors with NO exceptions export default ({
input: '...',
warnings: 'error',
output: { /* ... */ }
}); An open question would be how people would learn about the correct error codes. At the moment, the only sure way is to add an |
#2981 will fix the behaviour of |
#2981 was merged |
How Do We Reproduce?
https://rollupjs.org/repl?version=0.67.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMGElMjBmcm9tJTIwJy4lMkZhJyUzQiU1Q24lNUNuYS55KCU1QyUyMmFiYyU1QyUyMiklMjIlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIyYS5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjAlN0IlN0QlM0IlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyaWlmZSUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=
Expected Behavior
Reporting an error for not being able to find
y
and fail the build.Actual Behavior
Success build and no error being reported when
--silent
is set, as the error message is being reported throughonwarn
.The text was updated successfully, but these errors were encountered: