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
Missing exports errors now print the importing module #3401
Missing exports errors now print the importing module #3401
Conversation
I have made this initial PR to enable discussion. If we're happy with the change, and would like it see it merged, I'll circle back and fix test failures. I'm knocking this out during my workday, and don't want to burn that time just yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely something I would merge if you would fix the tests
Am circling back to get these tests working. In my local NodeJS v10.15.1 environment, I'm getting 3 failing tests on
Is this something that other contributors are hitting? I'm going to focus on failures other than these 3, and will rely on CircleCI to guide me. |
d5f9cae
to
72c9197
Compare
Codecov Report
@@ Coverage Diff @@
## master #3401 +/- ##
=======================================
Coverage 93.29% 93.29%
=======================================
Files 172 172
Lines 6112 6112
Branches 1822 1822
=======================================
Hits 5702 5702
Misses 218 218
Partials 192 192
Continue to review full report at Codecov.
|
@lukastaegert I've refactored to use All tests are passing in CI, though I'm still getting failures locally for:
I'd be interested in your thoughts on whether this is an ignoreable issue, or whether it suggests my local machine (Node 10 on macOS Catalina) will have issues running rollup in watch mode. As it stands, I have been using Rollup's watch mode with no problems. Thanks for your time helping me with this |
Prior to this change, we only printed the imported module that failed, with no context on which module was attempting the import. This could be difficult to debug in large codebases, especially when importing commonly-used modules.
The phrasing attempts to match the "imported by" style elsewhere in the codebase
72c9197
to
9c8764c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, looks great! Regarding your test failures, as those tests are using fs.watch
while the equivalent chokidar
tests are running, I would not worry too much. Rollup@2 should be just a few days away and it will switch to using chokidar
watching for everyone, making this issue irrelevant.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Resolves #3400
Description
This PR refactors the error message printed by
Module.handleMissingExport
, so it now contains the importing module.Prior to this change, we only printed the imported module that failed, with no context on which module was attempting the import. In some cases, the error could occur due to an invalid import within the importing module, so this info may needed to remedy the problem. This could be difficult to debug in large codebases, especially when importing commonly-used modules.