Skip to content
This repository has been archived by the owner on Dec 17, 2023. It is now read-only.

Add a fix mode that automatically fixes unused exports #67

Closed
wants to merge 9 commits into from

Conversation

phiresky
Copy link
Contributor

No description provided.

@phiresky
Copy link
Contributor Author

Also adds exit-status cmd option. That's really useful and a tiny change, @nadeesha if you're not sure about this one I can make a separate PR for that.

@nadeesha
Copy link
Owner

Thank you for this. Since this has a huge impact on the functionality, please allow me some time to review this and gather my thoughts. Appreciate your patience on this.

@phiresky
Copy link
Contributor Author

I used this to find and remove almost 80 dead exports with 4000 dead lines total from a 200kLOC code base. It's not perfect and you have to iterate multiple times, and use it in combination with noUnusedLocals, but it's still very useful and much faster than going through them by hand. It also doesn't affect the other functionality in any way, but I guess it is a chunk of maintenance effort if other people start using it :)

@nadeesha
Copy link
Owner

@phiresky Doing exit status on another PR would be great.

Also, I don't doubt how useful this is. And I always maintained that I'll accept a PR if someone's willing to do the work (which you have - Thanks!). It's just that we need to be really careful about the safeguards in the wild as someone could potentially lose uncommitted code if the fix malfunctions.

One option is to only run it on a non-dirty git HEAD.

I'll need to think about this a little bit, so please bear with me.

@phiresky
Copy link
Contributor Author

Right, didn't think of that. Yes, I think most tools that modify code check for vcs (mostly just git probably) status before modifying code.

This is probably enough:

const execFile = require('util').promisify(require('child_process').execFile;

const {stdout} = await execFile("git", ["status", "--short", "--porcelain"], {cwd: require("path").dirname("path_to_tsconfig.json")});

const dirty = stdout.length > 0;

Or something from here: https://stackoverflow.com/a/2659808/2639190

@phiresky
Copy link
Contributor Author

Also, I guess even that wouldn't be fully safe since the tsconfig could indirectly reference a file outside of the git repository. A check could be added to only modify files if they are "below" the tsconfig.json, but that could still have false negative and not sure if that's worth it.

@nadeesha
Copy link
Owner

@phiresky Sorry about the delay. I've been thinking about this, and I think this is a valuable feature.

I think I'm ready to accept this, if there was some way in which we can assert that the file being modified is not dirty. If asserting the files which are outside of the current working directory (process.cwd()) is hard, we can skip those files and require the user to run ts-prune from those directories specifically.

If these sound reasonable to you, then I'm more than willing to accept this contribution with the modifications. If you need help in this regard, please shout out.

I'm worried about the possible data loss and I'd like to aggressively guard against it.

@JustFly1984
Copy link

We are using TS-prune in gatsby.js project. Gatsby does not support typescript in config files, so some of TS files imported in config js files, and also gatsby has templates which is imported in node environment with require(), and currently TS-prune count templates as unused exports.

A bit off topic, but I think related question:

Is it possible to detect TS files which has no exports at all? For example files which has all the code commented out?

@nadeesha
Copy link
Owner

nadeesha commented Sep 7, 2020

@phiresky Since it's been a while, do you have an idea on how to proceed with this?

@phiresky
Copy link
Contributor Author

phiresky commented Sep 7, 2020

@nadeesha in my opinion adding the code I mentioned above of executing git status in the tsconfig dir is enough, even though it technically isn't guaranteed to cover all files in the project. What do you think? I haven't gotten to implementing that and I wasn't sure if you thought it sufficient, though I'm still using this fork in our CI.

@phiresky
Copy link
Contributor Author

phiresky commented Sep 7, 2020

@JustFly1984

Gatsby does not support typescript in config files, so some of TS files imported in config js

In our (isomorphic) project we have a few different cases where ts-prune has false positives:

  • Stuff used only in local config files that are git ignored - this then works locally but fails in clean environments like a CI
  • Files imported via dynamic / wildcard import() s in webpack, esp. bundles that are loaded within a react component
  • Files executed directly via pm2 (ts entry points)
  • Files imported from js with ts-node
  • Etc

We solved these by just adding Ts-prune-ignore comments to all of these with a specific reasoning, took a few iterations to get it all correct without accidentally removing used code but now it's fine.

I dont think adding support for other kinds of imports is realistic (except for cases supported by the TS language service itself, but I guess those should already work for projects with allowJS enabled and the is files included in the tsconfig)

Regarding empty files you should probably open a more specific different issue since I don't really understand what you mean

@nadeesha
Copy link
Owner

nadeesha commented Sep 7, 2020

@phiresky

in my opinion adding the code I mentioned above of executing git status in the tsconfig dir is enough, even though it technically isn't guaranteed to cover all files in the project. What do you think? I haven't gotten to implementing that and I wasn't sure if you thought it sufficient, though I'm still using this fork in our CI.

I think executing the git status is enough. Thanks for checking. If you do that, we can make this work.

@nadeesha
Copy link
Owner

I'm going to close this due to inactivity.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants