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.

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

Merged
merged 4 commits into from Mar 6, 2020

Conversation

timiyay
Copy link
Contributor

@timiyay timiyay commented Feb 25, 2020

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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.

@timiyay
Copy link
Contributor Author

timiyay commented Feb 25, 2020

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.

Copy link
Member

@lukastaegert lukastaegert left a 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

src/Module.ts Outdated Show resolved Hide resolved
@timiyay
Copy link
Contributor Author

timiyay commented Feb 28, 2020

Am circling back to get these tests working. In my local NodeJS v10.15.1 environment, I'm getting 3 failing tests on master:

rollup.watch
      fs.watch
        1) stops watching files that are no longer part of the graph
        2) ignores files that are not specified in options.watch.include, if given
        3) ignores files that are specified in options.watch.exclude, if given

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.

@timiyay timiyay force-pushed the improved-import-error-messaging branch from d5f9cae to 72c9197 Compare February 28, 2020 22:41
@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #3401 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
src/Module.ts 97.53% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baf382a...9c8764c. Read the comment docs.

@timiyay
Copy link
Contributor Author

timiyay commented Feb 28, 2020

@lukastaegert I've refactored to use relativeId and changed the relevant test expectations. When you have a spare moment, could you please review?

All tests are passing in CI, though I'm still getting failures locally for:

rollup.watch - fs.watch - ignores files that are specified in options.watch.exclude, if given
rollup.watch - fs.watch - ignores files that are not specified in options.watch.include, if given:

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

Timothy Asquith added 4 commits March 6, 2020 11:39
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
@timiyay timiyay force-pushed the improved-import-error-messaging branch from 72c9197 to 9c8764c Compare March 6, 2020 00:40
Copy link
Member

@lukastaegert lukastaegert left a 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.

@lukastaegert lukastaegert merged commit 1e6284f into rollup:master Mar 6, 2020
@timiyay timiyay deleted the improved-import-error-messaging branch March 6, 2020 05:30
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.

Improved error messaging for missing export errors
2 participants