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

Use local lerna if available #1122

Merged
merged 10 commits into from
Jan 19, 2018

Conversation

marionebl
Copy link
Contributor

@marionebl marionebl commented Nov 17, 2017

Description

This will change the CLI behaviour to use lerna from the nearest node_modules relative to process.cwd() if available.

Motivation and Context

How Has This Been Tested?

  • Additional unit tests for cli

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.

Breaking Change:

lerna now prefers installs resolvable relative to the current working directory.

For situations where a global install was used and an outdated lerna version
is present in the node_modules folder, this might cause breakage.

The issue can be resolved by installing a lerna version that fulfills the version
requirement in lerna.json

closes lerna#138

BREAKING CHANGE
lerna now prefers installs resolvable relative to the current working directory.
For situations where a global install was used and an outdated lerna version
is present in the node_modules folder, this might cause breakage.
The issue can be resolved by installing a lerna version that fulfills the version
requirement in lerna.json
@marionebl marionebl force-pushed the feat/use-local-lerna-if-available branch from cd55ee6 to 5af9075 Compare November 17, 2017 08:18
Copy link
Member

@evocateur evocateur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on!

bin/lerna.js Outdated
@@ -1,4 +1,6 @@
#!/usr/bin/env node
"use strict";
const resolveCwd = require("resolve-cwd");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the (more recent) import-local pattern, which encapsulates the tricky stuff for us:

const importLocal = require("import-local");

if (importLocal(__filename)) {
    require("npmlog").verbose("cli", "using local version of lerna");
} else {
    require("../lib/cli")().parse(process.argv.slice(2));
}

test/cli.js Outdated
it("should throw without command", async () => {
try {
await bin([]);
fail(`Expected an exception, but it didn"t throw anything`); // eslint-disable-line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does fail come from? Seems a little odd to templatize a string that isn't interpolated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fail is exposed by jasmine. I changed those to match the jest recommendations: https://facebook.github.io/jest/docs/en/tutorial-async.html#async-await

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's why I didn't recognize it. I really wish we could upgrade to Jest 21, but something in Jest 20 broke how we run the integration tests...

@evocateur
Copy link
Member

So, about the "breaking change"-ness...

I don't see this change as breaking for existing calls from npm scripts, which is (especially in CI) by far the most common usage? I agree that it does constitute a very different flow for those that call lerna directly in their terminal. Semver is hard. 😛

I suppose I should suck it up and sketch out a 3.0 milestone, bundling a few more breaking changes (#174, ...?).

@marionebl
Copy link
Contributor Author

Yeah, scratched my head quite a bit on categorizing this, too.

I tend to play it safe with semver and mark changes as breaking as soon as there is potential breakage, especially in uncommon usage scenarios (e.g. global installation, even on CI).

My personal approach to major releases is to handle them in no special way, so I'd release this without waiting for other changes. Def. your call though.

@qfox
Copy link

qfox commented Nov 17, 2017

If people using lerna as a CLI with some API and you breaking this API then it's breaking. If you don't breaking this API — then it's not.
Just decide what supported API is for this moment.

upd: Same for CI, btw.
upd2: Actually, lerna was recommended to be installed globally so the people who installed too old version of lerna locally (unsupported) will be affected. The rest will meet corrected behaviour when the local version (same as in CI?) will be used. So it's more like patch for me ;-)

@evocateur evocateur mentioned this pull request Nov 21, 2017
20 tasks
marionebl added a commit to marionebl/lerna that referenced this pull request Nov 21, 2017
@evocateur evocateur added this to the v3.0.0 milestone Jan 9, 2018
@lock
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
@marionebl marionebl deleted the feat/use-local-lerna-if-available branch December 27, 2018 08:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants