Throw friendly error when workspaces are not configured correctly

[craigbilner]

Description

If you add useWorkspaces to your lerna config but don't make the necessary changes in your package.json, lerna bootstrap will fail here.

Motivation and Context

This PR adds a validation error to make it clearer where the problem is, rather than pointing at the call to some on an undefined value for example.

Potentially it should throw if not an Array<string> too?

How Has This Been Tested?

There didn't appear to be any repo workspace tests so added an example for future use using the naming convention that seemed to exist. Added one test to confirm workspaces were correctly returned as a default and another for when useWorkspaces is configured only.

As an aside all tests run and pass for me if I use yarn test but yarn test:watch fails on several of them. I can look at this separately if needed?

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.

[evocateur]

Thanks, this is a good idea! A few formatting nits, and we're good to go.

[evocateur]

I think we can move this into the one test case that actually needs it, instead of copying over the directory fixture every single test.

-    it("returns workspace packageConfigs", () => {
+    it("returns workspace packageConfigs", async () => {
+      const testDirWithWorkspaces = await initFixture("Repository/yarn-workspaces");
       const repo = new Repository(testDirWithWorkspaces);

[evocateur]

I think we can avoid a solitary snapshot with a concise .toThrow(/workspaces need to be defined/) assertion here.

[evocateur]

And if you use the "EWORKSPACES" prefix, the regex can be even simpler: .toThrow(/EWORKSPACES/)

[evocateur]

This should be "EWORKSPACES", to be consistent with other ValidationErrors

[evocateur]

This should be a multiline tagged template string (using dedent, several examples of usage in src/Command.js).

Should probably call them "yarn workspaces", too, to clarify their origin.

[craigbilner]

@evocateur I've tidied it up

[evocateur]

Thanks!

[evocateur]

I’m not sure what’s up with the test:watch script, I just use yarn jest --watch most of the time. If you’re interested in figuring it out, go for it, but it’s not high priority.

[craigbilner]

@evocateur fyi - looks like it's to do with the way the tests borrow each others files so the tests can't run in parallel. Running in band works. Would be a larger rewrite of the tests.

[evocateur]

Ah, yes, that is a problem right now. Thanks for looking into it.

…

On Jan 18, 2018, at 08:27, Craig Bilner ***@***.***> wrote: @evocateur fyi - looks like it's to do with the way the tests borrow each others files so the tests can't run in parallel. Running in band works. Would be a larger rewrite of the tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[lock]

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.