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.

Sign up for GitHub

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

Jump to bottom

fix(core): more detailed error message when version cannot be found #3424

Merged
merged 3 commits into from Dec 6, 2022
Merged

fix(core): more detailed error message when version cannot be found #3424

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e780bb
fix(command): more detailed error message when version cannot be found
amorscher Nov 11, 2022
dbd869c
consider review comments
amorscher Dec 4, 2022
4b4e0ce
Merge branch 'lerna:main' into improve-error-messages
amorscher Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 30 additions & 0 deletions core/command/__tests__/command.test.js
View file
Open in desktop
Expand Up @@ -388,6 +388,36 @@ describe("core-command", () => {
);
});

it("throws ENOVERSION when lerna.json is empty", async () => {
const cwd = await initFixture("basic");

const lernaConfigPath = path.join(cwd, "lerna.json");
await fs.writeJson(lernaConfigPath, {});

await expect(testFactory({ cwd })).rejects.toThrow(
expect.objectContaining({
prefix: "ENOVERSION",
})
);
});

it("throws ENOVERSION when no version property exists in lerna.json", async () => {
const cwd = await initFixture("basic");

const lernaConfigPath = path.join(cwd, "lerna.json");
const lernaConfig = await fs.readJson(lernaConfigPath);
delete lernaConfig.version;
await fs.writeJson(lernaConfigPath, {
...lernaConfig,
});

await expect(testFactory({ cwd })).rejects.toThrow(
expect.objectContaining({
prefix: "ENOVERSION",
})
);
});

it("throws ENOWORKSPACES when npm client is pnpm and useWorkspaces is not true", async () => {
const cwd = await initFixture("pnpm");

Expand Down
6 changes: 5 additions & 1 deletion core/command/index.js
View file
Open in desktop
Expand Up @@ -240,10 +240,14 @@ class Command {
throw new ValidationError("ENOPKG", "`package.json` does not exist, have you run `lerna init`?");
}

if (!this.project.version) {
if (this.project.configNotFound) {
throw new ValidationError("ENOLERNA", "`lerna.json` does not exist, have you run `lerna init`?");
}

if (!this.project.version) {
throw new ValidationError("ENOVERSION", "Required property version does not exist in `lerna.json`");
}

if (this.options.independent && !this.project.isIndependent()) {
throw new ValidationError(
"EVERSIONMODE",
Expand Down
2 changes: 2 additions & 0 deletions core/project/index.js
View file
Open in desktop
Expand Up @@ -66,6 +66,7 @@ class Project {
// No need to distinguish between missing and empty,
Copy link
Member

Choose a reason for hiding this comment

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

Obviously I can't speak to the intent behind this comment originally, but your change does make it seem a little nonsensical, given you are adding an isEmpty right below a comment saying "no need to distinguish between missing and empty".

We could probably just amend the this.configNotFound assignment to check for Object.keys(loaded.config ?? {}).length === 0?

Copy link
Contributor Author

@amorscher amorscher Dec 4, 2022 •
edited

Choose a reason for hiding this comment

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

You are right that is a little bit weird. For this case i think I need to distinguish btw. emtpy and notFound because in case of empty the error message of no lerna version should appear.
The comment is related to the following line assigning an emtpy object to config. This make completly sense there because doing this avoids nasty if checks. I changed the flag from empty to noConfigFound and return it in the config. Hope this makes it a little bit less nonsensical and understandable. What do you think?

// saves a lot of noisy guards elsewhere
config: {},
isEmpty: true,
// path.resolve(".", ...) starts from process.cwd()
filepath: path.resolve(cwd || ".", "lerna.json"),
};
Expand Down Expand Up @@ -96,6 +97,7 @@ class Project {

/** @type {ProjectConfig} */
this.config = loaded.config;
this.configNotFound = loaded.isEmpty;
this.rootConfigLocation = loaded.filepath;
this.rootPath = path.dirname(loaded.filepath);

Expand Down