[package-json-lint] Add rule for hard coded dependencies in module and devModule. #704

pranav300 · 2021-09-01T14:15:04Z

Summary

This PR adds the rule require-no-hard-coded-dependency-versions, to not allow any hardcoded dependencies in module or devModule.

Closes: #697

Additional Details

@cerner/terra

package.json

ryanthemanuel · 2021-09-01T15:39:16Z

packages/package-json-lint-config-terra/package-json-lint.config.js

@@ -2,5 +2,6 @@ module.exports = {
  rules: {
    'require-no-terra-base-peer-dependency-versions': 'warn',
    'require-theme-context-versions': 'warn',
+    'require-no-hard-coded-dependency-versions': 'warn',


I actually think we should start this as an error out of the gate.

Updated here cc6d1e0.

But now the build will fail due to hardcoded dependencies in these packages:
eslint-config-terra
jest-config-terra
terra-function-testing

I tried updating these packages with no hard-coded dependencies.
At least in terra-toolkit only eslint-config-terra is causing a fail (Note: Currently we are using 7.22.0). As the latest released version of [email protected], throws an error no prop-validation of children prop. But that will be fixed in either 7.25.2 or 7.26.0, as per their changelog eslint-plugin-react

Ahh I see. I think in the long run, we'll want consumers to be able to allow for these types of situations. Do you think it would be possible to allow for something like this in the config?

"package-json-lint": {
"extends": "package-json-lint.config",
"projectType": "devModule",
"rules": {
"require-no-hard-coded-dependency-versions": ["error", { "allowList": "eslint-plugin-react" }]
}
},

What I am thinking is, we can change the structure of severity for all the rules to something like this:

"require-no-hard-coded-dependency-versions": {"severityType": "error", "allowList": ["eslint-plugin-react"] }

This will allow us to have more consistent structure for all the rules. What do you think?

Let's go with your suggestion. What I had suggested was something that eslint does, but I think we can go our own way with this.

Updated here dfc30a5

packages/package-json-lint/CHANGELOG.md

benbcai · 2021-09-02T16:06:23Z

packages/package-json-lint/src/rules/require-no-hard-coded-dependency-versions.js

+        const currentProblems = Object.keys(dependencies).map(dependencyName => {
+          const dependencyVersion = dependencies[dependencyName];
+          if (!dependencyVersion.startsWith('^') && !(ruleConfig.severity.allowList && ruleConfig.severity.allowList.includes(dependencyName))) {
+            return `${dependencyName}@${dependencyVersion} does not satisfy requirement for ${messageString}`;


Should we update messageString to call out which rule it does not satisfy instead of no hard coded dependency?

${dependencyName}@${dependencyVersion} does not satisfy requirement for the require-no-hard-coded-dependency-versions rule;

We should also add documentation for each rule explaining why we set it up and what problems it helps teams avoid.

Should we update messageString to call out which rule it does not satisfy instead of no hard coded dependency?

Updated here b3446c5.

We should also add documentation for each rule explaining why we set it up and what problems it helps teams avoid.

This is what I was thinking for the documentation.

Added documentation here 0095a1f

packages/package-json-lint/src/rules/require-no-hard-coded-dependency-versions.js

benbcai · 2021-09-02T18:57:39Z

packages/package-json-lint/src/rules/require-no-hard-coded-dependency-versions.js

+        const messageString = 'no hard coded dependency';
+        const currentProblems = Object.keys(dependencies).map(dependencyName => {
+          const dependencyVersion = dependencies[dependencyName];
+          if (!dependencyVersion.startsWith('^') && !(ruleConfig.severity.allowList && ruleConfig.severity.allowList.includes(dependencyName))) {


Some project dependencies may look like this that could reference a range, the latest snapshot, or renaming the dependency where ^ is not always the first character. Is the recommendation for teams to just add such dependencies in the allowList or should the rule handle some of these less common cases?

"blood-loss-calculator-js": ">= 2.3.0-dev.0 <2.3.0-dev.x"

"ion-provider-relationship-js": "master"

"procedures-js": "dev"

"real-wicg-inert": "npm:wicg-inert@^3.0.2"

I have added a compatibility regex array, through which we can check whether the dependency is hardcoded or not. Added here b3446c5.

Do let me know if I missed any scenarios. Thanks

mjhenkes · 2021-09-02T19:42:55Z

packages/jest-config-terra/package.json

@@ -40,7 +40,7 @@
    "@jest/reporters": "^25.3.0",
    "babel-jest": "^26.6.3",
    "identity-obj-proxy": "^3.0.0",
-    "jest-environment-jsdom": "26.6.2",
+    "jest-environment-jsdom": "^26.6.2",


I could not tell you why i locked this down, or if it was a mistake. 🤷‍♂️

ryanthemanuel · 2021-09-14T18:27:04Z

packages/package-json-lint/src/rules/require-no-hard-coded-dependency-versions.js

+        const messageString = 'require-no-hard-coded-dependency-versions';
+        const currentProblems = Object.keys(dependencies).map(dependencyName => {
+          const dependencyVersion = dependencies[dependencyName];
+          const isCompatibleVersion = compatibleVersionRegexList.map(versionRegex => (!!dependencyVersion.match(versionRegex))).includes(true);


I did a little digging with semver. I wonder if we can say:

const isCompatibleVersion = semver.clean(version) === null;

It doesn't look like semver.clean can handle ranges so syntax like ">= 2.3.0-dev.0 <2.3.0-dev.x" or even just 2.3.0-dev.x would not work so we may still need to handle those separately.

I think it’ll handle them fine. We want them to return undefined or null. Basically anything but a solitary version should return null or undefined by the clean function. Thus they will be accepted

@benbcai,
semver.clean works for >= 2.3.0-dev.0 <2.3.0-dev.x. And it will work if it has < > symbols.
It only doesn't work for 2.3.0-dev.x as this seems to be a fixed version.

If we are sure about it's going to be dev.x or branch.x, we can add it as an exception and have a check for it using regex.
What do you think? @ryanthemanuel @benbcai

2.3.0-dev.x will only resolve to a version that is released as 2.3.0-dev.x so this is effectively hard coded. I think it's fine to not allow this.

e.g. 1.0.0-rc.x does not actually resolve to 1.0.0-rc.1

From https://semver.npmjs.com/

I confused myself at first but after thinking thru this again, it all makes sense now.

Removed extension from the import.

….com/cerner/terra-toolkit into add-rule-for-hard-coded-dependencies

packages/terra-functional-testing/CHANGELOG.md

Co-authored-by: Akarsh Shetty <[email protected]>

added new rule for hard coded dependencies

e33f311

pranav300 requested review from ryanthemanuel and ShettyAkarsh September 1, 2021 14:15

pranav300 self-assigned this Sep 1, 2021

mjhenkes temporarily deployed to terra-toolki-add-rule-f-4zewih September 1, 2021 14:15 Inactive

pranav300 added 📦 package-json-lint 📦 package-json-lint-config-terra labels Sep 1, 2021

minor change

6c60eb4

mjhenkes temporarily deployed to terra-toolki-add-rule-f-4zewih September 1, 2021 14:26 Inactive

ryanthemanuel reviewed Sep 1, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

ryanthemanuel reviewed Sep 1, 2021

View reviewed changes

updated projectType to devModule for toolkit

cc6d1e0

mjhenkes temporarily deployed to terra-toolki-add-rule-f-4zewih September 1, 2021 18:17 Inactive

Added jest tests

841d4f9

mjhenkes temporarily deployed to terra-toolki-add-rule-f-4zewih September 1, 2021 19:16 Inactive

pranav300 marked this pull request as ready for review September 1, 2021 19:18

pranav300 requested review from mjhenkes and tbiethman as code owners September 1, 2021 19:18

pranav300 requested review from lokesh-0813, saket2403 and supreethmr September 1, 2021 19:19

minor change

ad34194

mjhenkes temporarily deployed to terra-toolki-add-rule-f-4zewih September 1, 2021 19:20 Inactive

Added allowList to the severity object

dfc30a5

mjhenkes temporarily deployed to terra-toolki-add-rule-f-4zewih September 2, 2021 15:24 Inactive

benbcai reviewed Sep 2, 2021

View reviewed changes

packages/package-json-lint/CHANGELOG.md Outdated Show resolved Hide resolved

benbcai reviewed Sep 2, 2021

View reviewed changes

packages/package-json-lint/src/rules/require-no-hard-coded-dependency-versions.js Outdated Show resolved Hide resolved

benbcai reviewed Sep 2, 2021

View reviewed changes

mjhenkes reviewed Sep 2, 2021

View reviewed changes

minor changes

80c4eb1

mjhenkes deployed to terra-toolki-add-rule-f-4zewih September 3, 2021 09:51 View deployment

ryanthemanuel reviewed Sep 14, 2021

View reviewed changes

Added documentation for package-json-lint

0095a1f

mjhenkes had a problem deploying to terra-toolki-add-rule-f-4zewih September 15, 2021 16:53 Failure

updated changelog

bc28dc4

mjhenkes had a problem deploying to terra-toolki-add-rule-f-4zewih September 15, 2021 17:53 Failure

minor changes

1f1cc1b

mjhenkes had a problem deploying to terra-toolki-add-rule-f-4zewih September 15, 2021 18:16 Failure

minor chnages

2b4a8d3

mjhenkes temporarily deployed to terra-toolki-add-rule-f-4zewih September 16, 2021 06:24 Inactive

mjhenkes temporarily deployed to terra-toolki-add-rule-f-wbdaol September 17, 2021 18:21 Inactive

Update Rules.2.tool.mdx

f7e974f

Removed extension from the import.

mjhenkes temporarily deployed to terra-toolki-add-rule-f-mp2gk0 September 17, 2021 19:57 Inactive

pranav300 added 2 commits September 20, 2021 19:51

minorChanges

d6206c3

Merge branch 'add-rule-for-hard-coded-dependencies' of https://github…

a3783ca

….com/cerner/terra-toolkit into add-rule-for-hard-coded-dependencies

mjhenkes temporarily deployed to terra-toolki-add-rule-f-mp2gk0 September 20, 2021 14:21 Inactive

ShettyAkarsh reviewed Sep 21, 2021

View reviewed changes

packages/terra-functional-testing/CHANGELOG.md Outdated Show resolved Hide resolved

Update packages/terra-functional-testing/CHANGELOG.md

d828bdd

Co-authored-by: Akarsh Shetty <[email protected]>

mjhenkes temporarily deployed to terra-toolki-add-rule-f-mp2gk0 September 21, 2021 19:17 Inactive

updated condition to use semver.clean

bce14c4

mjhenkes temporarily deployed to terra-toolki-add-rule-f-mp2gk0 September 22, 2021 12:58 Inactive

benbcai approved these changes Sep 22, 2021

View reviewed changes

ShettyAkarsh approved these changes Sep 22, 2021

View reviewed changes

Merge branch 'main' into add-rule-for-hard-coded-dependencies

74b9ac7

mjhenkes temporarily deployed to terra-toolki-add-rule-f-mp2gk0 September 23, 2021 04:34 Inactive

ryanthemanuel approved these changes Sep 23, 2021

View reviewed changes

supreethmr approved these changes Sep 24, 2021

View reviewed changes

ryanthemanuel merged commit 96aab2c into main Sep 27, 2021

ryanthemanuel deleted the add-rule-for-hard-coded-dependencies branch September 27, 2021 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[package-json-lint] Add rule for hard coded dependencies in module and devModule. #704

[package-json-lint] Add rule for hard coded dependencies in module and devModule. #704

pranav300 commented Sep 1, 2021 •

edited

Loading

ryanthemanuel Sep 1, 2021

pranav300 Sep 1, 2021

pranav300 Sep 1, 2021

ryanthemanuel Sep 1, 2021

pranav300 Sep 2, 2021 •

edited

Loading

ryanthemanuel Sep 2, 2021

pranav300 Sep 2, 2021

benbcai Sep 2, 2021

mjhenkes Sep 2, 2021

pranav300 Sep 3, 2021

pranav300 Sep 13, 2021

pranav300 Sep 15, 2021

benbcai Sep 2, 2021 •

edited

Loading

pranav300 Sep 3, 2021 •

edited

Loading

mjhenkes Sep 2, 2021

ryanthemanuel Sep 14, 2021

benbcai Sep 15, 2021 •

edited

Loading

ryanthemanuel Sep 22, 2021 •

edited

Loading

pranav300 Sep 22, 2021

ryanthemanuel Sep 22, 2021

ryanthemanuel Sep 22, 2021

benbcai Sep 22, 2021

[package-json-lint] Add rule for hard coded dependencies in module and devModule. #704

[package-json-lint] Add rule for hard coded dependencies in module and devModule. #704

Conversation

pranav300 commented Sep 1, 2021 • edited Loading

Summary

Additional Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranav300 Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbcai Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

pranav300 Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbcai Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

ryanthemanuel Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranav300 commented Sep 1, 2021 •

edited

Loading

pranav300 Sep 2, 2021 •

edited

Loading

benbcai Sep 2, 2021 •

edited

Loading

pranav300 Sep 3, 2021 •

edited

Loading

benbcai Sep 15, 2021 •

edited

Loading

ryanthemanuel Sep 22, 2021 •

edited

Loading