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

chore: add command to show unused exports #426

Closed
wants to merge 5 commits into from

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Oct 20, 2019

Summary

Invoke ts-prune with yarn ts:prune to output unused exports in the console. The library name and its description is a misnomer as it doesn't remove anything but if there's a short, easy to remember thing instead of ts:prune it can change, name suggestions are welcome!

Checklist

  • [ ] Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

@monfera monfera self-assigned this Oct 20, 2019
@monfera monfera added the chore label Oct 20, 2019
@codecov-io
Copy link

codecov-io commented Oct 20, 2019

Codecov Report

Merging #426 into master will increase coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #426      +/-   ##
=========================================
+ Coverage   77.85%   78.1%   +0.24%     
=========================================
  Files          81      91      +10     
  Lines        4209    4320     +111     
  Branches      894     872      -22     
=========================================
+ Hits         3277    3374      +97     
- Misses        920     933      +13     
- Partials       12      13       +1
Impacted Files Coverage Δ
src/mocks/series/series.ts 81.96% <0%> (ø)
src/mocks/specs/specs.ts 96% <0%> (ø)
src/mocks/scale/index.ts 100% <0%> (ø)
src/mocks/index.ts 100% <0%> (ø)
src/mocks/series/index.ts 100% <0%> (ø)
src/mocks/series/data.ts 100% <0%> (ø)
src/mocks/scale/scale.ts 87.5% <0%> (ø)
src/mocks/utils.ts 100% <0%> (ø)
src/mocks/series/utils.ts 88.88% <0%> (ø)
src/mocks/specs/index.ts 100% <0%> (ø)

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 8afa62a...79bce0f. Read the comment docs.

@markov00
Copy link
Member

I can see the potential of this, but there are a lot of false negative to deal with:

  • everything exported by the library, on our src/index.ts should be removed from the output
  • if a function is exported and not used (except the one exported as part of the library from the tsconfig) it means that this function is not tested (jest requires an exported function to work correctly).
  • it seems to report multiple time the same export, if it was first exported on it's own file, than from an index.ts

I think we have to wait for that until this nadeesha/ts-prune#14 is solved first

@monfera
Copy link
Contributor Author

monfera commented Oct 22, 2019

No rush on this, we can wait for the fix, though it seems to not cause harm and I like an explicit enlisting of useful if imperfect log commands in package.json as I'm bad at remembering incantations esp. after a vacation 😃and Ctrl-R isn't ideal either. Such tools are never perfect (incl. tslint whose lack of concern about unused exports was the motivation) - btw. I might check out this library as well

if a function is exported and not used (except the one exported as part of the library from the tsconfig) it means that this function is not tested (jest requires an exported function to work correctly)

My intent wasn't to automatically cull exports, just to see if there are exports that need some action, such as either removing the exporting, removing the entire export const/function ... (if not used) or knowing where to add a test case, at least as a placeholder for real tests. So I saw it as a positive, despite its imperfections like:

everything exported by the library, on our src/index.ts should be removed from the output

Good issue find in the ts-prune repo! I simply skimmed the output as these were typically top-level obvious things on the top. Wondering though, would it be possible to add the shallowest test case to cover the lib exports, even without exercising the API much? Maybe it's advantageous to have tests from the "outside" viewpoint which mimics how our library is used

@nickofthyme
Copy link
Collaborator

jenkins retest this please

@nickofthyme
Copy link
Collaborator

jenkins retest this please

@markov00 markov00 mentioned this pull request Nov 26, 2019
2 tasks
@monfera
Copy link
Contributor Author

monfera commented Nov 27, 2019

Closing it in favor of Marco's PR #461

@monfera monfera closed this Nov 27, 2019
@monfera monfera deleted the ts-prune branch November 27, 2019 09:24
markov00 added a commit that referenced this pull request Nov 28, 2019
Updated the following dev dependencies: babel, @elastic/eui, semantic-release, typescript-eslint, eslint, husky. Added ts-prune
close #426
@markov00
Copy link
Member

markov00 commented Dec 2, 2019

🎉 This issue has been resolved in version 15.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants