[package-json-lint] Add new rule require-dependencies-declared-at-appropriate-level #710
Conversation
…dd-rule-require-no-unnecessary-dependencies
pranav300
requested review from
supreethmr,
lokesh-0813,
saket2403 and
ShettyAkarsh
pranav300
requested review from
mjhenkes,
ryanthemanuel and
tbiethman
as code owners
ryanthemanuel
temporarily deployed
to
terra-toolki-add-rule-r-r6ja94
ryanthemanuel
temporarily deployed
to
terra-toolki-add-rule-r-r6ja94
| @@ -0,0 +1,131 @@ | |||
| const devDependencySet = [ | |||
There was a problem hiding this comment.
I have taken this set from devDependencies in packages in terra-toolkit and terra-core.
Please let me know if there are any packages that need to be added or removed.
Thanks.
There was a problem hiding this comment.
can we have a separate file for this ? as the list might grow.
There was a problem hiding this comment.
I don't think that is required as later many rules may have large sets. And having a different file for each of them and maintaining them seems unnecessary.
Let me know if you think otherwise?
There was a problem hiding this comment.
Created a seperate file dev-dependency-set.json here 71a07f7.
ryanthemanuel
temporarily deployed
to
terra-toolki-add-rule-r-r6ja94
…dd-rule-require-no-unnecessary-dependencies
| 'svgo', | ||
| 'terra-application', | ||
| 'terra-button', | ||
| 'terra-disclosure-manager', |
There was a problem hiding this comment.
What is the reason that these terra components, lodash, commander, etc must be a dev dependency? I would think many consumers are currently have these as regular dependencies.
There was a problem hiding this comment.
I had run a script to get dev and peer dependencies from the mono-repo's. And in that some packages have these as both dependencies and devDependencies. I will go through them once and remove the unneeded dependencies from the set.
Edit: I have removed some of the packages from the set here d8f833e.
| } | ||
| } | ||
| }, | ||
| peerDependencies: (peerDependencies) => { |
There was a problem hiding this comment.
Both dependencies and peerDependencies do pretty much the same thing. Is it possible to consolidate their implementations to make it more reusable for both?
There was a problem hiding this comment.
Updated here d8f833e.
| 'bufferutil', | ||
| 'chai', | ||
| 'check-installed-dependencies', | ||
| 'core-js', |
There was a problem hiding this comment.
I would think core-js can be a regular dependency. There are several other dependencies here that I'm not familiar with but ones like glob and postcss should be allowed as a regular dependency right?
There was a problem hiding this comment.
Sorry for going back and forth on this list. I think it is safe to keep the ones that we know with certainty that should be a dev dependencies, especially ones used for testing. For example, chai, sinon, mocha, lerna, gh-pages. I have seen projects adding wdio-mocha-framework as a regular dependency so anything wdio-* should be a dev dependency.
There was a problem hiding this comment.
I just saw Ryan's comment above. We can start small for now and add as go.
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| * Added | |||
| * Added new rule `require-no-unnecessary-dependency`. | |||
There was a problem hiding this comment.
Can we rename this rule? require-dependencies-declared-at-appropriate-level? Something like that would be a little clearer i think.
There was a problem hiding this comment.
Renamed the rule here 71a07f7
ryanthemanuel
had a problem deploying
to
terra-toolki-add-rule-r-r6ja94
pranav300
changed the title
packages/terra-toolkit-docs/src/terra-dev-site/tool/package-json-lint/Rules.2.tool.mdx
Outdated
Show resolved
Hide resolved
…on-lint/Rules.2.tool.mdx Co-authored-by: Ryan Manuel <[email protected]>
…dd-rule-require-no-unnecessary-dependencies
…//github.com/cerner/terra-toolkit into add-rule-require-no-unnecessary-dependencies
Summary
This PR adds a new rule require-no-unnecessary-dependency.
This rule doesn't allow unnecessary dependencies/peerDependencies, that can be shifted to devDependendies.
This rule is not applicable on
devModule.Closes #695
Additional Details
@cerner/terra