-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
0b64c84
to
a97192b
Compare
test/GitVersionParser.js
Outdated
}); | ||
}); | ||
|
||
describe("parseVersion - with prefix", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor]: I would rename with prefix
into with versionPrefix
. The title confused me with the prefix
property returned by the parser.
test/Package.js
Outdated
expect(mockSerializer.serialize.mock.calls.length).toBe(0); | ||
}); | ||
|
||
it("should call 'deserialize' on old and and 'serialize' on new serializer'", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] double and
test/PackageGraph.js
Outdated
expect(graph.get(pkg2.name).dependencies).toEqual([pkg1.name]); | ||
}); | ||
|
||
it(".get should not return the dependencies for urecognized versions", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] unrecognized
test/PackageGraph.js
Outdated
}); | ||
|
||
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep consistency, add spaces before and after []
like in the test before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/PackageGraph.js
Outdated
expect(graph.get(pkg2.name).dependencies).toEqual([]); | ||
}); | ||
|
||
it(".get should not return the dependencies for urecognized versions", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] unrecognized
and spaces before and after []
11b4445
to
aa85501
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's spread these dependencies across multiple lines, just like npm formats it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the same stuff in various tests is fine on one line)
src/commands/PublishCommand.js
Outdated
@@ -600,6 605,7 @@ export default class PublishCommand extends Command { | |||
}); | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray newline
src/commands/PublishCommand.js
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/commands/PublishCommand.js
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triple backticks are meaningless in JS, just use a dedent
-tagged template literal for a multiline string. (example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and really, just copy that example's throwing, don't worry about an explicit this.logger.error
call)
src/Package.js
Outdated
|
||
this._versionSerializer = versionSerializer; | ||
|
||
if (previousSerializer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this ever happen? Just being defensive?
(It seems strange to assign the versionSerializer multiple times)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's just being defensive. Currently, it is not likely that second serializer would be used in one run. YAGNI and remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
src/Repository.js
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
});
});
}
src/Repository.js
Outdated
if (this.lernaJson.useGitVersion) { | ||
packages.forEach((pkg) => { | ||
pkg.versionSerializer = new VersionSerializer({ | ||
monorepoDependencies: packageGraph.get(pkg.name).dependencies, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this cloneDeep
into the version serializer methods? It seems overly defensive for the non-serialized case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, there's a couple of reasons why this is done like this:
toJSON
was returning its internalpkg
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)
src/Repository.js
Outdated
@@ -118,8 120,22 @@ export default class Repository { | |||
} | |||
|
|||
buildPackageGraph() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- in Command, right after the
buildPackageGraph()
is called - 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;
@evocateur thanks for a detailed review, much appreciated! I will get back to you once the changes are done. |
e96918f
to
e88357c
Compare
Golly, I really need to fix that AppVeyor node v4 build. :/ Looking great so far! |
e88357c
to
1aad956
Compare
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. |
@gustaff-weldon Superb, thank you! Don't worry about rebasing, I will squash merge. |
Awesome @gustaff-weldon! I can't wait to try this out! |
@mattbrunetti kudos also to @synaptiko, we worked on that together. |
This is awesome! Thanks @gustaff-weldon and @synaptiko! |
Thanks for the heads up @ramasilveyra This is fantastic news indeed! |
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.. |
For the future reference, there are multiple ways how to achieve "git-only" distribution from monorepo. We've tried both of those:
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 @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). |
@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? |
@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 |
We now simply recognize any local package name with a matching git url. refs lerna#1033
* 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
As a side tip, I'm experimenting with this tool with git hooks/husky: 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 |
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. |
Description
This PR introduces optional support for package dependency version to be stored as url to git hosted repositories in
package.json
eg. givenpackage-1
that depends onpackage-2
, apackage.json
of the former might look like:package.json
file contains aserialized
version of package dependency versions, as full urls, that are read internally as semver ones ie. eg.0.0.1
.useGitVersion
andgitVersionPrefix
options inlerna.json
to enable the feature and optionally configure itVersionSerializer
that handles deserializing/serializing dependency version from/topackage.json
andGitVersionParser
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 ofpackage.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:
committish
partpublish
command should be used with--exact
since using^
or~
in commitish part is not supportedHow 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
Checklist:
run ci
)