Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for git-hosted urls as sibling package dependencies #1033

Merged
merged 4 commits into from Oct 5, 2017

Conversation

@gustaff-weldon
Copy link
Contributor

gustaff-weldon commented Sep 29, 2017

Description

This PR introduces optional support for package dependency version to be stored as url to git hosted repositories in package.json eg. given package-1 that depends on package-2, a package.json of the former might look like:

// packages/package-1/package.json
{
  version: "0.0.1",
  ...
  dependencies: {
    "package-2": "git://github.com/user/package-2.git#v0.0.1"
  }
}
  • implementation is based around the idea that package.json file contains a serialized version of package dependency versions, as full urls, that are read internally as semver ones ie. eg. 0.0.1.
  • uses useGitVersion and gitVersionPrefix options in lerna.json to enable the feature and optionally configure it
    {
      "lerna": "2.2.0",
      "packages": [
        "packages/*"
      ],
      "version": "0.0.1",
      "useGitVersion": true,
      "gitVersionPrefix": "v"
    } 
    
  • introduces VersionSerializer that handles deserializing/serializing dependency version from/to package.json and GitVersionParser to handle git-specific urls.

Motivation and Context

Lerna assumes packages to be distributed on npm upon publish. However, when private repo is not an option, packages can be distributed on private GH repositories.

We run a private Github Lerna monorepo with Ember addons and we publish our packages to separate read-only repositories, from which addons are installable separately (and can bring in other subpackages as dependencies).

With versions written down as 0.0.1 installation of git-hosted downstream repositories would fail when resolving other package dependencies. Storing urls in GH forms solves the issues, also keeps history and tags to point to an identical version of package.json files in monorepo and read-only repositories.

This PR shares the work we've done with @synaptiko to streamline this process. We've done our best to make it least invasive. We are open to improvement suggestions to make it even more suitable for inclusion in Lerna.

Known limitations:

  • feature requires dependency url to contain committish part
  • publish command should be used with --exact since using ^ or ~ in commitish part is not supported

How Has This Been Tested?

We run code from this PR internally to bootstrap and publish our monorepo. We have also updated Lerna tests to cover added code.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed. (we checked with run ci)
@gustaff-weldon gustaff-weldon force-pushed the tbayaa:git-version-support branch from 0b64c84 to a97192b Sep 29, 2017
});
});

describe("parseVersion - with prefix", () => {

This comment has been minimized.

Copy link
@Calanthe

Calanthe Sep 29, 2017

[minor]: I would rename with prefix into with versionPrefix. The title confused me with the prefix property returned by the parser.

expect(mockSerializer.serialize.mock.calls.length).toBe(0);
});

it("should call 'deserialize' on old and and 'serialize' on new serializer'", () => {

This comment has been minimized.

Copy link
@Calanthe

Calanthe Sep 29, 2017

[minor] double and

expect(graph.get(pkg2.name).dependencies).toEqual([pkg1.name]);
});

it(".get should not return the dependencies for urecognized versions", () => {

This comment has been minimized.

Copy link
@Calanthe

Calanthe Sep 29, 2017

[minor] unrecognized

});

it(".get should not return the dependencies for urecognized versions", () => {
const [pkg1, pkg2] = createPackages("0.0.1", "github:user-foo/project-foo#v0.0.1");

This comment has been minimized.

Copy link
@Calanthe

Calanthe Sep 29, 2017

To keep consistency, add spaces before and after [] like in the test before.

This comment has been minimized.

Copy link
@Calanthe

Calanthe Sep 29, 2017

Also, I am not sure if you need this test if you have the second .get should not return the dependencies for urecognized versions more detailed test.

expect(graph.get(pkg2.name).dependencies).toEqual([]);
});

it(".get should not return the dependencies for urecognized versions", () => {

This comment has been minimized.

Copy link
@Calanthe

Calanthe Sep 29, 2017

[minor] unrecognized and spaces before and after []

@gustaff-weldon gustaff-weldon force-pushed the tbayaa:git-version-support branch 2 times, most recently from 11b4445 to aa85501 Sep 29, 2017
Copy link
Member

evocateur left a comment

This PR was well-constructed from the get-go, please don't take the number of (mostly minor) comments as implicit criticism! I appreciate the care and attention to detail already in evidence.

Beyond stylistic issues, I think my biggest concern is keeping consistent with our "durable" options pattern (allowing CLI flags to override "embedded" config in lerna.json). As rare as I expect this particular pattern to be, I want to keep these configuration options as consistent as possible.

Thanks!

README.md Outdated
@@ -663,6 +663,8 @@ Running `lerna` without arguments will show all commands/options.
}
},
"packages": ["packages/*"]
"useGitVersion": true,

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

These aren't going to be very common, and should be documented separately (perhaps after --use-workspaces). Putting them here just confuses beginners.

README.md Outdated
name: "my-package-1",
version: "1.0.0",
bin: "bin.js",
dependencies: { "my-package-2": "github:example-user/my-package-2#1.0.0" },

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

let's spread these dependencies across multiple lines, just like npm formats it.

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

(the same stuff in various tests is fine on one line)

@@ -600,6 +605,7 @@ export default class PublishCommand extends Command {
});
}


This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

stray newline

@@ -156,6 +156,11 @@ export default class PublishCommand extends Command {
this.gitRemote = this.options.gitRemote || "origin";
this.gitEnabled = !(this.options.canary || this.options.skipGit);

if (this.repository.lernaJson.useGitVersion && !this.options.exact) {

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

useGitVersion should be referenced from this.options, which supports CLI flag overrides.

That being said, if --use-git-version requires --exact, we should throw an error here.

@@ -156,6 +156,11 @@ export default class PublishCommand extends Command {
this.gitRemote = this.options.gitRemote || "origin";
this.gitEnabled = !(this.options.canary || this.options.skipGit);

if (this.repository.lernaJson.useGitVersion && !this.options.exact) {
this.logger.error("config", ```Using git version without 'exact' option is not recommended.

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

Triple backticks are meaningless in JS, just use a dedent-tagged template literal for a multiline string. (example)

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

(and really, just copy that example's throwing, don't worry about an explicit this.logger.error call)


this._versionSerializer = versionSerializer;

if (previousSerializer) {

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

When does this ever happen? Just being defensive?

(It seems strange to assign the versionSerializer multiple times)

This comment has been minimized.

Copy link
@gustaff-weldon

gustaff-weldon Oct 4, 2017

Author Contributor

Yes, it's just being defensive. Currently, it is not likely that second serializer would be used in one run. YAGNI and remove?

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 4, 2017

Member

Yes, please.

this._packageGraph = PackageUtilities.getPackageGraph(this._packages);
const packages = PackageUtilities.getPackages(this);
const packageGraph = PackageUtilities.getPackageGraph(packages, false, this.lernaJson.useGitVersion
? new GitVersionParser(this.lernaJson.gitVersionPrefix)

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

I'd prefer the version parser be instantiated outside of the getPackageGraph() arguments, this ternary initially made me think you had forgotten the closing paren. Also, the version parser could be reused in the subsequent version serializer loop.

const { useGitVersion, gitVersionPrefix } = this.lernaJson;
// FIXME: should be destructured from command.options to support CLI flag overrides

const versionParser = useGitVersion && new GitVersionParser(gitVersionPrefix);
const packages = PackageUtilities.getPackages(this);
const packageGraph = PackageUtilities.getPackageGraph(packages, false, versionParser);

if (useGitVersion) {
  packages.forEach((pkg) => {
    pkg.versionSerializer = new VersionSerializer({
      monorepoDependencies: packageGraph.get(pkg.name).dependencies,
      versionParser,
    });
  });
}
if (this.lernaJson.useGitVersion) {
packages.forEach((pkg) => {
pkg.versionSerializer = new VersionSerializer({
monorepoDependencies: packageGraph.get(pkg.name).dependencies,

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

graphDependencies sounds clearer to me, here. It's not the dependencies of the entire monorepo, after all.

isPrivate() {
return !!this._package.private;
}

toJSON() {
return this._package;
const pkg = _.cloneDeep(this._package);

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

Can we move this cloneDeep into the version serializer methods? It seems overly defensive for the non-serialized case.

This comment has been minimized.

Copy link
@gustaff-weldon

gustaff-weldon Oct 4, 2017

Author Contributor

We can, there's a couple of reasons why this is done like this:

  • toJSON was returning its internal pkg object, any outside changes on it will affect Package
  • cloning only for serializer case would make toJSON behave differently - in one case it will return its internal object the other time it would return a deep copy

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 4, 2017

Member

Despite .toJSON() currently being used only in one place, I think you've raised some good points that will ultimately help future use cases (such as when I actually get around to refactoring lerna into a lerna-managed monorepo and the Package class is exposed in a package of its own). I was being overly-cautious about performance in a place where it really, really doesn't matter (package.json files aren't particularly large, and not deeply nested at all).

Thanks for the clear explanation. :)

@@ -118,8 +120,22 @@ export default class Repository {
}

buildPackageGraph() {

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 2, 2017

Member

We should pass an options object derived from this.options when it is called from Command.js and use those properties instead of this.lernaJson. This allows us to support CLI flags (--use-git-version, --git-version-prefix) and eventually argument validation with yargs.

diff --git a/src/Command.js b/src/Command.js
index e5bb0c4..032cd01 100644
--- a/src/Command.js
+++ b/src/Command.js
@@ -288,7 +288,7 @@ export default class Command {
   }
 
   runPreparations() {
-    const { scope, ignore, registry, since } = this.options;
+    const { scope, ignore, registry, since, useGitVersion, gitVersionPrefix } = this.options;
 
     if (scope) {
       log.info("scope", scope);
@@ -303,7 +303,7 @@ export default class Command {
     }
 
     try {
-      this.repository.buildPackageGraph();
+      this.repository.buildPackageGraph({ useGitVersion, gitVersionPrefix });
       this.packages = this.repository.packages;
       this.packageGraph = this.repository.packageGraph;
       this.filteredPackages = PackageUtilities.filterPackages(this.packages, { scope, ignore });

(Sorry for the scattered review)

This comment has been minimized.

Copy link
@gustaff-weldon

gustaff-weldon Oct 4, 2017

Author Contributor

Any suggestion what would be the best way to deal with buildPackageGraph being called from
packages and packageGraph getters?

Could we remove those implicit calls? I do not see any functionality depending on that getter behaviour atm.
We could also set current command options on the repository and then pass them:

// Command.js
this.repository.setCurrentCommandOptions(this.options);

But I have mixed feelings about that. Unless there's already some way to access currently running command from the repository?

This comment has been minimized.

Copy link
@evocateur

evocateur Oct 4, 2017

Member

Yeah, I totally understand the mixed feelings. I have them myself, about the Repository class in general. It's doing too many disparate things for my taste, but it's been low on the priority list to refactor...

So: It occurs to me that Repository itself shouldn't be storing packages and packageGraph in getters since there are literally two places in the entire codebase that they are referenced:

  1. in Command, right after the buildPackageGraph() is called
  2. in UpdatedPackagesCollector, where it violates separation of concern by reaching into (sub-)properties of the command instance just to retrieve mostly-static (i.e., unchanging) values.

Here's what we can do: Move the creation of both the packages list and the packageGraph into Command, where it belongs, and completely remove the get packages and get packageGraph from Repository.

This means all the additional logic added here will be contained inside the place where this.options already exists. Something like this:

diff --git a/src/Command.js b/src/Command.js
index 58659bb..aa4bc7f 100644
--- a/src/Command.js
+++ b/src/Command.js
@@ -294,7 +294,8 @@ export default class Command {
   }
 
   runPreparations() {
-    const { scope, ignore, registry, since } = this.options;
+    const { rootPath, packageConfigs } = this.repository;
+    const { scope, ignore, registry, since, useGitVersion, gitVersionPrefix } = this.options;
 
     if (scope) {
       log.info("scope", scope);
@@ -309,10 +310,22 @@ export default class Command {
     }
 
     try {
-      this.repository.buildPackageGraph();
-      this.packages = this.repository.packages;
-      this.packageGraph = this.repository.packageGraph;
-      this.filteredPackages = PackageUtilities.filterPackages(this.packages, { scope, ignore });
+      const versionParser = useGitVersion && new GitVersionParser(gitVersionPrefix);
+      const packages = PackageUtilities.getPackages({ rootPath, packageConfigs });
+      const packageGraph = PackageUtilities.getPackageGraph(packages, false, versionParser);
+
+      if (useGitVersion) {
+        packages.forEach((pkg) => {
+          pkg.versionSerializer = new VersionSerializer({
+            graphDependencies: packageGraph.get(pkg.name).dependencies,
+            versionParser,
+          });
+        });
+      }
+
+      this.packages = packages;
+      this.packageGraph = packageGraph;
+      this.filteredPackages = PackageUtilities.filterPackages(packages, { scope, ignore });
 
       // The UpdatedPackagesCollector requires that filteredPackages be present prior to checking for
       // updates. That's okay because it further filters based on what's already been filtered.
diff --git a/src/Repository.js b/src/Repository.js
index 8316083..7f1e138 100644
--- a/src/Repository.js
+++ b/src/Repository.js
@@ -8,7 +8,6 @@ import semver from "semver";
 
 import dependencyIsSatisfied from "./utils/dependencyIsSatisfied";
 import Package from "./Package";
-import PackageUtilities from "./PackageUtilities";
 
 const DEFAULT_PACKAGE_GLOB = "packages/*";
 
@@ -67,22 +66,6 @@ export default class Repository {
       .map(parentDir => path.resolve(this.rootPath, parentDir));
   }
 
-  get packages() {
-    if (!this._packages) {
-      this.buildPackageGraph();
-    }
-
-    return this._packages;
-  }
-
-  get packageGraph() {
-    if (!this._packageGraph) {
-      this.buildPackageGraph();
-    }
-
-    return this._packageGraph;
-  }
-
   get packageJson() {
     if (!this._packageJson) {
       try {
@@ -117,11 +100,6 @@ export default class Repository {
     return this.version === "independent";
   }
 
-  buildPackageGraph() {
-    this._packages = PackageUtilities.getPackages(this);
-    this._packageGraph = PackageUtilities.getPackageGraph(this._packages);
-  }
-
   hasDependencyInstalled(depName, version) {
     log.silly("hasDependencyInstalled", "ROOT", depName, version);
 
diff --git a/src/UpdatedPackagesCollector.js b/src/UpdatedPackagesCollector.js
index 67d8ba5..5e51b60 100644
--- a/src/UpdatedPackagesCollector.js
+++ b/src/UpdatedPackagesCollector.js
@@ -31,7 +31,7 @@ export default class UpdatedPackagesCollector {
     this.logger = command.logger;
     this.repository = command.repository;
     this.packages = command.filteredPackages;
-    this.packageGraph = command.repository.packageGraph;
+    this.packageGraph = command.packageGraph;
     this.options = command.options;
   }
 
diff --git a/test/Repository.js b/test/Repository.js
index 1c21bfc..8b01475 100644
--- a/test/Repository.js
+++ b/test/Repository.js
@@ -142,32 +142,6 @@ describe("Repository", () => {
     });
   });
 
-  describe("get .packages", () => {
-    it("returns the list of packages", () => {
-      const repo = new Repository(testDir);
-      expect(repo.packages).toEqual([]);
-    });
-
-    it("caches the initial value", () => {
-      const repo = new Repository(testDir);
-      expect(repo.packages).toBe(repo.packages);
-    });
-  });
-
-  describe("get .packageGraph", () => {
-    it("returns the graph of packages", () => {
-      const repo = new Repository(testDir);
-      expect(repo.packageGraph).toBeDefined();
-      expect(repo.packageGraph).toHaveProperty("nodes", []);
-      expect(repo.packageGraph).toHaveProperty("nodesByName", {});
-    });
-
-    it("caches the initial value", () => {
-      const repo = new Repository(testDir);
-      expect(repo.packageGraph).toBe(repo.packageGraph);
-    });
-  });
-
   describe("get .packageJson", () => {
     afterEach(() => {
       readPkg.sync = readPkgSync;
@gustaff-weldon

This comment has been minimized.

Copy link
Contributor Author

gustaff-weldon commented Oct 3, 2017

@evocateur thanks for a detailed review, much appreciated! I will get back to you once the changes are done.

@gustaff-weldon gustaff-weldon force-pushed the tbayaa:git-version-support branch 2 times, most recently from e96918f to e88357c Oct 4, 2017
@evocateur

This comment has been minimized.

Copy link
Member

evocateur commented Oct 4, 2017

Golly, I really need to fix that AppVeyor node v4 build. :/

Looking great so far!

@gustaff-weldon gustaff-weldon force-pushed the tbayaa:git-version-support branch from e88357c to 1aad956 Oct 5, 2017
@gustaff-weldon

This comment has been minimized.

Copy link
Contributor Author

gustaff-weldon commented Oct 5, 2017

I have updated the code. Please let me know if something can be further improved. If accepted, I will rebase this PR into one commit.

@evocateur

This comment has been minimized.

Copy link
Member

evocateur commented Oct 5, 2017

@gustaff-weldon Superb, thank you! Don't worry about rebasing, I will squash merge.

@evocateur evocateur changed the title Introduced support for git-hosted dependency urls of packages Add support for git-hosted urls as sibling package dependencies Oct 5, 2017
@evocateur evocateur merged commit 5b8795c into lerna:master Oct 5, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mattbrunetti

This comment has been minimized.

Copy link

mattbrunetti commented Oct 5, 2017

Awesome @gustaff-weldon! I can't wait to try this out!

@gustaff-weldon

This comment has been minimized.

Copy link
Contributor Author

gustaff-weldon commented Oct 6, 2017

@mattbrunetti kudos also to @synaptiko, we worked on that together.
I'm glad you find this useful!

@ramasilveyra

This comment has been minimized.

Copy link

ramasilveyra commented Oct 6, 2017

This is awesome! Thanks @gustaff-weldon and @synaptiko!

@jamiehill

This comment has been minimized.

Copy link

jamiehill commented Oct 7, 2017

Thanks for the heads up @ramasilveyra

This is fantastic news indeed!

@mattbrunetti

This comment has been minimized.

Copy link

mattbrunetti commented Oct 11, 2017

Did not end up using this :( Could not find resources/support on the workflow of this "git-only" way of distributing packages.. Decided to try out npm private packages instead..

@synaptiko

This comment has been minimized.

Copy link

synaptiko commented Oct 11, 2017

For the future reference, there are multiple ways how to achieve "git-only" distribution from monorepo.

We've tried both of those:

  1. splitsh
  2. regular git filter-branch --subdirectory-filter packages/<package>

Both are quite fast, splitsh is faster (we have less then 200 commits though) and both require to have prepared "downstream" repositories in advance. There is one bummer with filter-branch related to tags. You have to either restore original tags after the push of the "split". Or you have to create a fresh clone of the repository before each split.

@mattbrunetti It was easier for us to setup this than to manage private npm repository (which requires every developer to setup his npm/yarn accordingly).

@chinchang

This comment has been minimized.

Copy link

chinchang commented Jan 9, 2018

@synaptiko I have one major confusion regarding all this. Please help me clear it:

"This PR only allows putting versions as git urls. That's all. Pushing individual packages to downstream git repos has to be still handled on our own (using splitish etc)."

Am I correct on this?

@synaptiko

This comment has been minimized.

Copy link

synaptiko commented Jan 9, 2018

@chinchang yes, you are correct… I understand why it's confusing but unfortunately it would make lerna even more complex to support it directly… it also depends on what CI you use and so on

evocateur added a commit to evocateur/lerna that referenced this pull request Feb 20, 2018
evocateur added a commit to evocateur/lerna that referenced this pull request Feb 20, 2018
We now simply recognize any local package name with a matching git url.

refs lerna#1033
evocateur added a commit that referenced this pull request Feb 22, 2018
* Support gitRange (#semver:^1.2.3) as well as gitCommittish
* Move dependency update logic into Package#updateDependency()
* Test PublishCommand/normal-exact with meaningful fixtures

BREAKING CHANGE:

* Remove --use-git-version and --git-version-prefix options
  The former is now implied, and the latter is always "" or "v".

  Updating git hosted versions no longer throws when --exact is missing.

* Remove VersionSerializer, GitVersionParser
  We now simply recognize any local package name with a matching git url.

refs #1033
@minhuaF minhuaF mentioned this pull request Mar 20, 2018
@JacopKane

This comment has been minimized.

Copy link

JacopKane commented Aug 4, 2018

As a side tip, I'm experimenting with this tool with git hooks/husky:
https://github.com/ingydotnet/git-subrepo

Seems like doing the job well without the need to keep repositories read-only. The only gotcha was I needed to filter my git hooks for the only root origin in order to get rid of endless push/pulls

@lock

This comment has been minimized.

Copy link

lock bot commented Dec 27, 2018

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.