-
Notifications
You must be signed in to change notification settings - Fork 70
Add a fix mode that automatically fixes unused exports #67
Conversation
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. |
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. |
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 :) |
@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. |
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 |
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. |
@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 ( 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. |
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? |
@phiresky Since it's been a while, do you have an idea on how to proceed with this? |
@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. |
In our (isomorphic) project we have a few different cases where ts-prune has false positives:
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 |
I think executing the git status is enough. Thanks for checking. If you do that, we can make this work. |
I'm going to close this due to inactivity. |
No description provided.