Fix for no-unused-modules to handle the export { default } from syntax

[richardxia]

Relates to #1631.

At the moment, I only have a failing test case, since it took me a while to understand how imports and exports worked with the testing system. If my current understanding is correct, the filename property refers to the file that this test "replaces", and the code property is the text contents of the replacement.

The way I've proven to myself that this demonstrates the bug is that if I replace the contents of the code property with

`import o from './file-o'
export default o`

then the test passes. The export { default } from './file-o' syntax should be equivalent to the above, so if we get that test passing, then the bug should be fixed.

I'm going to make an attempt at understanding the ExportMap code enough to try to fix the bug, but I've checked the "Allow edits from maintainers" box on this PR, so feel free to push up commits to this.

[coveralls]

Coverage decreased (-4.1%) to 93.717% when pulling c538e69 on richardxia:1631-no-unused-modules-handle-export-default-from into bbd166b on benmosher:master.

[coveralls]

Coverage decreased (-4.1%) to 93.717% when pulling c538e69 on richardxia:1631-no-unused-modules-handle-export-default-from into bbd166b on benmosher:master.

[coveralls]

Coverage decreased (-4.1%) to 93.717% when pulling c538e69 on richardxia:1631-no-unused-modules-handle-export-default-from into bbd166b on benmosher:master.

[coveralls]

Coverage decreased (-4.1%) to 93.717% when pulling c538e69 on richardxia:1631-no-unused-modules-handle-export-default-from into bbd166b on benmosher:master.

[coveralls]

Coverage increased (+0.001%) to 97.797% when pulling efd6be1 on richardxia:1631-no-unused-modules-handle-export-default-from into adbced7 on benmosher:master.

[ljharb]

Test looks good; looking forward to the fix :-)

[richardxia]

I have a fix, but I'm not sure if it's correct or complete.

What bothers me is that I was struggling to get the tests passed until I realized that running my test in isolation would work, but running it with the other tests would cause it to fail. I don't currently really understand how the updateExportUsage and updateImportUsage work, nor do I understand how they interact with the test suite, where we sometimes replace a real test file with slightly different textual contents.

I also had to modify the one other test that I identified as interacting with my own test. This is the one where we basically have a copy of file-0.js embedded into the test itself. It appears that once per Mocha run, we parse all of the test files and construct the dependency graphs, but for each individual test case, we sometimes modify that dependency graph. Running the other test was causing the dependency between file-0.js and my new file-s.js to get removed, and it would never get restored. It seems to me that we should be purging pieces of the dependency graph that get stubbed out for testing pieces so that earlier tests don't interfere with later ones.

One general suggestion I have about the code within no-unused-modules.js is to better state what the invariants are before and after any of the important functions run. I had trouble understanding what the expected state of importList and exportList were after each of the functions, especially with respect to things like the handling of special-case behavior, such as mapping the exported name 'default' to 'ImportDefaultSpecifier'.

[richardxia]

Hi @ljharb, I was wondering if you had the chance to take a look at this.

I'm also up for refactoring or improving code comments/documentation of this plugin so that it is easier for future contributors to make changes to.

[maniator]

Is there any update on this PR?

This issue is blocking our team from using this eslint rule.

[richardxia]

I think this is ready for review. Let me update the PR title to indicate that it's more than just a failing test case, since there's a fix included as well, and that it's not WIP (at least from my perspective).

[richardxia]

I did a bit of cleaning up here including:

• Rebasing against the latest master
• Adding much more detailed comments describing the importList and exportList top-level variables: 9bd07e7. Please let me know if you think I've gone a bit overboard here, and I'd be happy to trim it down or even just revert that change.
• Adding a line to the CHANGELOG: 66a6a43

@ljharb, please let me know if there's anything else I can do here. Also, feel free to make any changes you want directly to this branch, since I've checked the "Allow edits from maintainers." box on this PR.

[maniator]

What is blocking this change?

This would help out our team immensely!

Great job on this @richardxia 😄!

[richardxia]

Thanks for merging this in, and I'm excited to use this lint rule on my codebase soon!

[maniator]

@ljharb -- is it possible to make a release of the plugin?

My team would really appreciate this fix :-)