fuels-ts icon indicating copy to clipboard operation
fuels-ts copied to clipboard

chore: Remove unused files and exports

Open maschad opened this issue 1 year ago • 5 comments

Running knip reports the following

unused files

Screenshot 2024-04-08 at 11 51 28 AM

unused exports

Screenshot 2024-04-08 at 11 51 57 AM

unused export types

Screenshot 2024-04-08 at 11 52 38 AM

I suspect there may be some more configuration to be done to get this to be more accurate.

Once #2024 is resolved this can be done.

maschad avatar Apr 08 '24 16:04 maschad

What is considered an unused export?

For instance, we don't want to stop exporting constants meant to be used externally.

arboleya avatar Apr 10 '24 19:04 arboleya

Agreed @arboleya I think we will have to add a bit of manual configuration around the unused export issue type to ensure we don't remove those. It will be quite a bit of grunt work upfront unfortunately.

maschad avatar Apr 10 '24 20:04 maschad

If I'm following the idea here (and on https://github.com/FuelLabs/fuels-ts/pull/2026), the introduction of knip is to automate and make up for [manual] things we can forget and may go unnoticed. However, considering the configuration exceptions will also be manual, can go outdated, and be somehow forgotten, will it still be an improvement over the current scenario?

arboleya avatar Apr 11 '24 10:04 arboleya

That's a good point, but I don't think that it applies to unused files or unresolved imports or duplicate exports, as we shouldn't seek to introduce anymore of those.

The manual configuration applies primarily to the unused exports issue type , but considering that in an ideal scenario, all the exported constants that would be consumed by external libraries would be distributed across constants.ts files, we could then rationalize that other constants would unused. As you rightly point out this will still be manual but I think it reduces the overall manual overhead considering the points mentioned above.

maschad avatar Apr 11 '24 16:04 maschad

Apologies in advance for my unsolicited feedback here.

What is considered an unused export?

For instance, we don't want to stop exporting constants meant to be used externally.

By default, exports of entry files are not reported as unused (this is configurable though). In other words, in this case, you could add constants.ts as an entry file and call it a day.

However, considering the configuration exceptions will also be manual, can go outdated, and be somehow forgotten, will it still be an improvement over the current scenario?

This is a great point. For example, ESLint disable-line comments, rules or even plugins might no longer be necessary at some point too. This type of maintenance is almost inevitable. Knip tries to be as unobtrusive as possible. For instance, as a solution for the same issue, exports can be JSDoc-tagged @public, serving as (internal) documentation at the same time.

AMA! There's some ways to go about configuration, I'm sure there's a way to knip it ✂️

webpro avatar Apr 16 '24 07:04 webpro

Thanks for creating this awesome library @webpro 🚀 , and also thanks for your input, really appreciate your direction on this 😄 . I think right now all the reported unused files and exports are misreported for us, and so the configuration would be disproportionately more than the size savings.

  • For instance BYTES_32 and a number of other constants are reported as Duplicate exports even though they aren't duplicates but rather re-used.

  • All these ErrorCodes are reported as unused as well, and we wouldn't want to set that file as our entry file, rather if it could determine that the fact it's exported via the index entry point that would be useful.

  • All the reported unused files are actually configuration files such as vitest test configs for example.

I think we can revisit this issue in the future as knip matures.

maschad avatar May 14 '24 21:05 maschad

Thanks a lot for the feedback, it was very helpful. Full disclosure:

  • https://x.com/webprolific/status/1790823813191713256
  • https://x.com/webprolific/status/1794995970561704446

Not trying to push anything here, just know that since v5.17 with a lot less configuration the results are a lot better:

{
  "workspaces": {
    "*/*": {
      "entry": ["src/{index,main}.{ts,tsx}", "fuels.config*.ts"]
    }
  },
  "ignoreDependencies": [
    "@fuel-ts/.+",
    "@graphql-codegen/.+",
    "graphql-tag",
    "events",
    "eslint-plugin-jsdoc",
    "memfs",
    "open",
    "textlint",
    "textlint-rule-no-dead-link",
    "@elasticpath/textlint-rule-no-dead-relative-link",
    "ts-generator",
    "webdriverio"
  ]
}

webpro avatar May 28 '24 15:05 webpro