Conversation
|
|
This is great, thank you! Whew |
.github/workflows/ts-packages.yml
Outdated
There was a problem hiding this comment.
I think it's reasonable to assume that all of the packages within packages will be npm/yarn based (we're putting Go packages in the go directory). In that case, is it possible to loop over each directory within a GitHub action instead of listing every directory explicitly? I haven't worked with GitHub actions enough to know offhand. That way this script would work automatically for new packages and we wouldn't have to remember to add the packages to this list of directories.
There was a problem hiding this comment.
Hmm, yeah I'm not finding a clean way to do this. There's the option to use a matrix, but I don't think it's possible to feed in a dynamic value there.
There was a problem hiding this comment.
I'll leave myself a reminder to add this to a doc that explains what needs to be modified when adding a new package.
integration-tests/.depcheckrc
Outdated
There was a problem hiding this comment.
I'll make a separate issue for reviewing these dependencies and making sure that they all belong in this ignore list.
package.json
Outdated
There was a problem hiding this comment.
Maybe we could do npx depcheck instead of installing it as a dependency here (depcheck seems to come with a lot of dependencies of its own and we try to avoid adding dependencies where possible), but that might be a bit incongruous with what we do elsewhere. cc @tynes what do you think?
There was a problem hiding this comment.
I agree that depcheck adds a ton of dependencies that we don't really need, for example the yarn.lock now has vue related dependencies... we would like to keep the attack surface as low as possible when it comes to adding in extra deps. Doing npx depcheck works as it will install it at runtime, it runs in CI so installing it there is fine. That would let us not need to include it in the package.json dependencies
There was a problem hiding this comment.
Good call. Just updated.
Codecov Report
@@ Coverage Diff @@
## develop #1989 +/- ##
===========================================
+ Coverage 74.42% 74.58% +0.16%
===========================================
Files 79 79
Lines 2545 2554 +9
Branches 397 401 +4
===========================================
+ Hits 1894 1905 +11
+ Misses 651 649 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
.github/workflows/ts-packages.yml
Outdated
There was a problem hiding this comment.
I'll leave myself a reminder to add this to a doc that explains what needs to be modified when adding a new package.
noahlitvin
force-pushed
the
feat/depcheck
branch
from
66431cf to
2b74b98
Compare
|
Yep! Should be all set @smartcontracts |
|
@noahlitvin sorry for the delay here! I'm going to run the tests one more time. Should be good to merge after that. If you'd like a contributor NFT send an ETH address to kelvin AT optimism DOT io 🙂. |
|
@noahlitvin would you mind rebasing this on top of develop one more time? Can get it merged after that. |
4 participants
Description
This PR adds depcheck to the
.github/workflows/ts-packages.ymlCI workflow to detect unused packages in the codebase.Additional context
The issue raised by @smartcontracts suggests relying on the
--ignore-pathoption to avoid errors being thrown from false positives (such as packages being relied on by Solidity smart contracts, undetected by depcheck). The ignore-path option specifies files to ignore, rather than packages. This won't help with censoring false positives for unused packages. (i.e. depcheck will see packages inpackage.jsonand not be able to recognize their use in Solidity files; ignoring the Solidity files won't change this.) Instead, I've used theignoresoption specified in.depcheckrcfiles.Metadata