Skip to content

Commit

Permalink
Noviny/enforce internal dev dep range (#24)
Browse files Browse the repository at this point in the history
* up is not down

* fix things

* Update .changeset/cyan-cups-push.md

Co-Authored-By: Mitchell Hamilton <[email protected]>

* Update .changeset/cyan-cups-push.md

Co-Authored-By: Mitchell Hamilton <[email protected]>

* Update README.md

Co-Authored-By: Mitchell Hamilton <[email protected]>

* Update .changeset/cyan-cups-push.md

Co-Authored-By: Mitchell Hamilton <[email protected]>

* updated messaging

* Update .changeset/cyan-cups-push.md
  • Loading branch information
Noviny authored Oct 23, 2019
1 parent 1dbb6d1 commit 86bd46d
Show file tree
Hide file tree
Showing 11 changed files with 254 additions and 22 deletions.
31 changes: 31 additions & 0 deletions .changeset/cyan-cups-push.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
"@manypkg/cli": minor
---

Add new check: INTERNAL_DEV_DEP_NOT_STAR

This check moves internal devDependencies between packages to be `*` - so in a case where I had a package sunshine, which depends on internal package 'sun':

```json
{
"name": "sunshine",
"version": "1.0.0",
"devDependencies": {
"sun": "^1.0.0"
}
}
```

we will now have:

```json
{
"name": "sunshine",
"version": "1.0.0",
"devDependencies": {
"sun": "*"
}
}
```

This is because all internal dependencies are always linked if the version of the internal dependency is within the specified range(which is already enforced by Manypkg), and devDependencies are only relevant in local installs. Having set versions here caused packages to be patched when one of their devDependencies left the range, which was not strictly necessary.
11 changes: 9 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,20 @@ The highest range of the dependency is set as the range at every non-peer depend

## Internal mismatch

The ranges for all dependencies(excluding `peerDependencies`) on internal packages should include the version of the internal package. This is so that an internal package will never depend on another internal package but get the package from the registry because that happening is very confusing and you should always prefer a local version of any given package.
The ranges for all regular dependencies and optionalDependencies(not peerDependencies or devDependencies) on internal packages should include the version of the internal package. This is so that an internal package will never depend on another internal package but get the package from the registry because that happening is very confusing and you should always prefer a local version of any given package.

### How it's fixed

If the range is a [caret range](https://github.com/npm/node-semver#caret-ranges-123-025-004) or a [tilde range](https://github.com/npm/node-semver#tilde-ranges-123-12-1) with no other comparators, the range is set as a caret or tilde range respectively with the version of the internal package. If it is any other range, the range is set to the exact version of the internal package.

## Internal devDependencies are not `*`

The ranges for devDependencies should not matter, and having non-star versions causes accidental patching of packages when no changes have been made.

### How it's fixed

Change whatever other version is specified to be `*`

## Invalid dev and peer dependency relationship

All `peerDependencies` should also be specified in `devDependencies` and the range specified in `devDependencies` should be a subset of the range for that dependency in `peerDependencies`. This is so that `peerDependencies` are available in the package during development for testing and etc.
Expand Down Expand Up @@ -85,4 +93,3 @@ When you add a package with `yarn add` or etc. dependencies are sorted, and this
#### How it's fixed

This is fixed by sorting deps by key alphabetically.

2 changes: 1 addition & 1 deletion packages/cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ yarn manypkg check
yarn manypkg fix
```

See more details at https://github.com/mitchellhamilton/manypkg
See more details at https://github.com/thinkmill/manypkg
6 changes: 3 additions & 3 deletions packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
"find-up": "^4.1.0",
"find-workspaces-root": "^0.1.0",
"fs-extra": "^8.1.0",
"get-workspaces": "^0.4.0",
"get-workspaces": "^0.5.0",
"meow": "^5.0.0",
"p-limit": "^2.2.1",
"sembear": "^0.4.0",
"sembear": "^0.4.1",
"semver": "^6.3.0",
"spawndamnit": "^2.0.0",
"validate-npm-package-name": "^3.0.0"
Expand All @@ -32,4 +32,4 @@
"script-runner"
]
}
}
}
43 changes: 43 additions & 0 deletions packages/cli/src/checks/INTERNAL_DEV_DEP_NOT_STAR.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { makeCheck, Workspace } from "./utils";

export type ErrorType = {
type: "INTERNAL_DEV_DEP_NOT_STAR";
workspace: Workspace;
dependencyWorkspace: Workspace;
};

export default makeCheck<ErrorType>({
validate: (workspace, allWorkspaces) => {
let errors: ErrorType[] = [];
let deps = workspace.config.devDependencies;
if (deps) {
for (let depName in deps) {
let range = deps[depName];
let dependencyWorkspace = allWorkspaces.get(depName);
if (dependencyWorkspace !== undefined && range !== "*") {
errors.push({
type: "INTERNAL_DEV_DEP_NOT_STAR",
workspace,
dependencyWorkspace
});
}
}
}

return errors;
},
fix: error => {
let deps = error.workspace.config.devDependencies;
if (deps && deps[error.dependencyWorkspace.name]) {
deps[error.dependencyWorkspace.name] = "*";
}
return { requiresInstall: true };
},
print: error =>
`${error.workspace.name} has a dependency on ${
error.dependencyWorkspace.name
} as a devDependency, but has the version listed as ${
error.workspace.config.devDependencies![error.dependencyWorkspace.name]
}. Please update the dependency to be "*"`,
type: "all"
});
5 changes: 4 additions & 1 deletion packages/cli/src/checks/INTERNAL_MISMATCH.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
} from "./utils";
import semver from "semver";

type ErrorType = {
export type ErrorType = {
type: "INTERNAL_MISMATCH";
workspace: Workspace;
dependencyWorkspace: Workspace;
Expand All @@ -17,6 +17,8 @@ export default makeCheck<ErrorType>({
validate: (workspace, allWorkspaces) => {
let errors: ErrorType[] = [];
for (let depType of NORMAL_DEPENDENCY_TYPES) {
// devDependencies are handled by INTERNAL_DEV_DEP_NOT_STAR
if (depType === "devDependencies") continue;
let deps = workspace.config[depType];
if (deps) {
for (let depName in deps) {
Expand All @@ -36,6 +38,7 @@ export default makeCheck<ErrorType>({
}
}
}

return errors;
},
fix: error => {
Expand Down
66 changes: 66 additions & 0 deletions packages/cli/src/checks/__tests__/INTERNAL_DEV_DEP_NOT_STAR.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import makeCheck, { ErrorType } from "../INTERNAL_DEV_DEP_NOT_STAR";
import { Workspace } from "get-workspaces";

let getFakeWS = (
name: string = "pkg-1",
version: string = "1.0.0"
): Workspace => {
return {
name,
dir: `some/fake/dir/${name}`,
config: {
name,
version
}
};
};

let getWS = (): Map<string, Workspace> => {
let ws = new Map();
ws.set("pkg-1", getFakeWS());
return ws;
};

describe("internal dev ep is not star", () => {
it("should not error if internal version is *", () => {
let ws = getWS();
let dependsOnOne = getFakeWS("depends-on-one");
dependsOnOne.config.devDependencies = {
"pkg-1": "*"
};
ws.set("depends-on-one", dependsOnOne);
let errors = makeCheck.validate(dependsOnOne, ws);
expect(errors.length).toEqual(0);
});
it("should error if internal version is not *", () => {
let ws = getWS();
let dependsOnOne = getFakeWS("depends-on-one");
dependsOnOne.config.devDependencies = {
"pkg-1": "^1.0.0"
};
ws.set("depends-on-one", dependsOnOne);
let errors = makeCheck.validate(dependsOnOne, ws);
expect(errors[0]).toMatchObject({
type: "INTERNAL_DEV_DEP_NOT_STAR",
workspace: dependsOnOne,
dependencyWorkspace: ws.get("pkg-1")
});
expect(errors.length).toEqual(1);
});
it("should fix an incompatible version", () => {
let workspace = getFakeWS("depends-on-one");
workspace.config.devDependencies = {
"pkg-1": "^1.0.0"
};
let error: ErrorType = {
type: "INTERNAL_DEV_DEP_NOT_STAR",
workspace,
dependencyWorkspace: getFakeWS()
};
let result = makeCheck.fix!(error);
expect(result).toMatchObject({ requiresInstall: true });
expect(error.workspace.config.devDependencies).toMatchObject({
"pkg-1": "*"
});
});
});
85 changes: 85 additions & 0 deletions packages/cli/src/checks/__tests__/INTERNAL_MISMATCH.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import makeCheck, { ErrorType } from "../INTERNAL_MISMATCH";
import { Workspace } from "get-workspaces";

let getFakeWS = (
name: string = "pkg-1",
version: string = "1.0.0"
): Workspace => {
return {
name,
dir: `some/fake/dir/${name}`,
config: {
name,
version
}
};
};

let getWS = (): Map<string, Workspace> => {
let ws = new Map();
ws.set("pkg-1", getFakeWS());
return ws;
};

describe("internal mismatch", () => {
it("should not error if internal version is compatible", () => {
let ws = getWS();
let dependsOnOne = getFakeWS("depends-on-one");
dependsOnOne.config.dependencies = {
"pkg-1": "^1.0.0"
};
ws.set("depends-on-one", dependsOnOne);
let errors = makeCheck.validate(dependsOnOne, ws);
expect(errors.length).toEqual(0);
});
it("should error if internal version is not compatible", () => {
let ws = getWS();
let dependsOnOne = getFakeWS("depends-on-one");
dependsOnOne.config.dependencies = {
"pkg-1": "^0.1.0"
};
ws.set("depends-on-one", dependsOnOne);
let errors = makeCheck.validate(dependsOnOne, ws);
expect(errors[0]).toMatchObject({
type: "INTERNAL_MISMATCH",
dependencyWorkspace: ws.get("pkg-1"),
workspace: dependsOnOne,
dependencyRange: "^0.1.0"
});
expect(errors.length).toEqual(1);
});
it("should fix an incompatible version", () => {
let workspace = getFakeWS("depends-on-one");
workspace.config.dependencies = {
"pkg-1": "^0.1.0"
};

let error: ErrorType = {
type: "INTERNAL_MISMATCH",
workspace,
dependencyWorkspace: getFakeWS(),
dependencyRange: "^0.1.0"
};

let fixed = makeCheck.fix!(error);
expect(fixed).toMatchObject({ requiresInstall: true });
expect(workspace.config.dependencies).toMatchObject({ "pkg-1": "^1.0.0" });
});
it("should not check dev dependencies", () => {
// This is handled by INTERNAL_DEV_DEP_NOT_STAR
let ws = getWS();
let dependsOnOne = getFakeWS("depends-on-one");
dependsOnOne.config.devDependencies = {
"pkg-1": "^0.1.0"
};
ws.set("depends-on-one", dependsOnOne);
let errors = makeCheck.validate(dependsOnOne, ws);
expect(errors.length).toEqual(0);
});
// NB this is the same as dependency right now, but I added it for completeness
describe.skip("peerDependency", () => {
it("should not error if internal version is compatible", () => {});
it("should error if internal version is not compatible", () => {});
it("should fix an incompatible version", () => {});
});
});
2 changes: 2 additions & 0 deletions packages/cli/src/checks/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import EXTERNAL_MISMATCH from "./EXTERNAL_MISMATCH";
import INTERNAL_MISMATCH from "./INTERNAL_MISMATCH";
import INTERNAL_DEV_DEP_NOT_STAR from "./INTERNAL_DEV_DEP_NOT_STAR";
import INVALID_DEV_AND_PEER_DEPENDENCY_RELATIONSHIP from "./INVALID_DEV_AND_PEER_DEPENDENCY_RELATIONSHIP";
import INVALID_PACKAGE_NAME from "./INVALID_PACKAGE_NAME";
import MULTIPLE_DEPENDENCY_TYPES from "./MULTIPLE_DEPENDENCY_TYPES";
Expand All @@ -9,6 +10,7 @@ import UNSORTED_DEPENDENCIES from "./UNSORTED_DEPENDENCIES";
export let checks = [
EXTERNAL_MISMATCH,
INTERNAL_MISMATCH,
INTERNAL_DEV_DEP_NOT_STAR,
INVALID_DEV_AND_PEER_DEPENDENCY_RELATIONSHIP,
INVALID_PACKAGE_NAME,
MULTIPLE_DEPENDENCY_TYPES,
Expand Down
3 changes: 0 additions & 3 deletions test-gatsby/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
"name": "test-gatsby-thing",
"private": true,
"version": "0.0.1",
"peerDependencies": {
"gatsby": "^2.15.28"
},
"dependencies": {
"@manypkg/gatsby-source-workspace": "0.1.0",
"@mdx-js/mdx": "^1.5.1",
Expand Down
22 changes: 10 additions & 12 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6130,14 +6130,6 @@ get-value@^2.0.3, get-value@^2.0.6:
resolved "https://registry.yarnpkg.com/get-value/-/get-value-2.0.6.tgz#dc15ca1c672387ca76bd37ac0a395ba2042a2c28"
integrity sha1-3BXKHGcjh8p2vTesCjlbogQqLCg=

get-workspaces@^0.4.0:
version "0.4.0"
resolved "https://registry.yarnpkg.com/get-workspaces/-/get-workspaces-0.4.0.tgz#512dedc0e9a96b0c17035af363236299247f4cc8"
integrity sha512-O4pSctjkmrLRs8GxnaQ/xy16PWoZpEUzpgtBAj9UIYNrtag1rRnFOyvJULIMPhpbZKOlswmrS5Ku6pjt35Yj+g==
dependencies:
fs-extra "^7.0.1"
globby "^9.2.0"

get-workspaces@^0.5.0:
version "0.5.0"
resolved "https://registry.yarnpkg.com/get-workspaces/-/get-workspaces-0.5.0.tgz#bc00b6d158f409a70ba54cc95e58bf29cfe4df1e"
Expand Down Expand Up @@ -6220,6 +6212,12 @@ global-prefix@^1.0.1:
version "1.0.2"
resolved "https://registry.yarnpkg.com/global-prefix/-/global-prefix-1.0.2.tgz#dbf743c6c14992593c655568cb66ed32c0122ebe"
integrity sha1-2/dDxsFJklk8ZVVoy2btMsASLr4=
dependencies:
expand-tilde "^2.0.2"
homedir-polyfill "^1.0.1"
ini "^1.3.4"
is-windows "^1.0.1"
which "^1.2.14"

global@^4.3.0:
version "4.4.0"
Expand Down Expand Up @@ -11881,10 +11879,10 @@ selfsigned@^1.10.7:
dependencies:
node-forge "0.9.0"

sembear@^0.4.0:
version "0.4.0"
resolved "https://registry.yarnpkg.com/sembear/-/sembear-0.4.0.tgz#19a140d659205069f5f9003edb9671d545ffa5c1"
integrity sha512-CHBII+H79ZAKU/85p4wS0JFTYuKCzzHffQ3NsLnMcpRjjx3ZcNE8CLuNa2QnxWDwXskg8ey8jJbfCOf5E2Lx4A==
sembear@^0.4.1:
version "0.4.1"
resolved "https://registry.yarnpkg.com/sembear/-/sembear-0.4.1.tgz#c1599bba0fd29cce68a25382576891e5fae84990"
integrity sha512-q3LJepi7qgDbkR9eDe1OVYfbD8A14vuC8TtETtEvpAQ4GxynA9B0fhG6ts8scBMDe/GwLlVUeU+Icnl1oidH7g==
dependencies:
"@types/semver" "^6.0.1"
semver "^6.3.0"
Expand Down

0 comments on commit 86bd46d

Please sign in to comment.