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

Jump to bottom

Add to UNUSED_EXTERNAL_IMPORT warning information about the origin of the problem #4054

Merged
merged 5 commits into from Apr 27, 2021
Merged

Add to UNUSED_EXTERNAL_IMPORT warning information about the origin of the problem #4054

merged 5 commits into from Apr 27, 2021

Conversation

cawa-93
Copy link
Contributor

@cawa-93 cawa-93 commented Apr 21, 2021

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:

Description

This patch add to UNUSED_EXTERNAL_IMPORT warning information about the origin of the problem

@github-actions
Copy link

github-actions bot commented Apr 21, 2021 •
edited

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install cawa-93/rollup#unused-external-imports-improvements

or load it into the REPL:
https://rollupjs.org/repl/?pr=4054

cawa-93
cawa-93 commented Apr 21, 2021
View reviewed changes
src/ExternalModule.ts
names: unused,
source: this.id
source: this.id,
sources: importersArray
Copy link
Contributor Author

@cawa-93 cawa-93 Apr 21, 2021 •
edited

Choose a reason for hiding this comment

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

I'm not sure how appropriate to use sources key. I did not find any description of the properties of the RollupWarning interface.

rollup/src/rollup/types.d.ts

Lines 9 to 22 in a4a3b8a

export interface RollupWarning extends RollupLogProps {
chunkName?: string;
cycle?: string[];
exporter?: string;
exportName?: string;
guess?: string;
importer?: string;
missing?: string;
modules?: string[];
names?: string[];
reexporter?: string;
source?: string;
sources?: string[];
}

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the other usages, sources should be reasonable.

@codecov
Copy link

codecov bot commented Apr 21, 2021 •
edited

Codecov Report

Merging #4054 (d5500f7) into master (a4a3b8a) will decrease coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4054      +/-   ##
==========================================
- Coverage   97.48%   97.45%   -0.03%     
==========================================
  Files         192      192              
  Lines        6788     6795       +7     
  Branches     1996     1996              
==========================================
+ Hits         6617     6622       +5     
  Misses         84       84              
- Partials       87       89       +2
Impacted Files Coverage Δ
src/ExternalModule.ts 96.22% <71.42%> (-3.78%) ⬇️

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 a4a3b8a...d5500f7. Read the comment docs.

@cawa-93 cawa-93 marked this pull request as ready for review April 21, 2021 14:11
lukastaegert
lukastaegert reviewed Apr 22, 2021
View reviewed changes
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.

Some things could likely be simplified, otherwise looks good

src/ExternalModule.ts Outdated
const {importers, dynamicImporters} = this.declarations[name].module

if (Array.isArray(importers)) importers.forEach(v => importersSet.add(v))
if (Array.isArray(dynamicImporters)) dynamicImporters.forEach(v => importersSet.add(v))
Copy link
Member

Choose a reason for hiding this comment

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

By the types, these checks should not be needed. This is also reflected in the coverage right now. Or do you see a scenario where there might be an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During my previous experiments there was a situation when dynamicImporters was undefined. I assume that the types are not quite correct. Therefore, for reinsurance, I added a check.

src/ExternalModule.ts Outdated Show resolved Hide resolved
src/ExternalModule.ts
names: unused,
source: this.id
source: this.id,
sources: importersArray
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the other usages, sources should be reasonable.

@lukastaegert lukastaegert merged commit 8c59abe into rollup:master Apr 27, 2021
@cawa-93 cawa-93 deleted the unused-external-imports-improvements branch April 27, 2021 20:07
This was referenced Jul 28, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guybedford guybedford guybedford approved these changes

@lukastaegert lukastaegert lukastaegert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add more information to UNUSED_EXTERNAL_IMPORT warning
3 participants
@cawa-93 @guybedford @lukastaegert