Add flow for monorepo w/ independent versions #6

mcmire · 2022-07-18T22:48:00Z

This commit introduces a rough cut of a flow which supports a monorepo
with an independent versioning strategy. This flow:

Generates a "release specification" file containing all of the
workspace packages within the repo (this will be changed to all
updated packages in a future commit)
Opens the user's editor (if one can be detected) and waits for them to
edit it
Parses the edited version of the release spec
Applies the release spec by:
- Updating the version of the root package with the current date
  (note: the format of the version string will be changed in a future
  commit)
- Updating the versions of all of the packages listed in the release
  spec
- Adds new sections to the changelogs of all of the packages listed in
  the release spec

v8 isn't quite reliable when using Node 14 for development (it has too many false negatives). We can consider changing this back if/when we switch to Node 16 or later, but for now we will stick to Babel.

This commit introduces a rough cut of a flow which supports a monorepo with an independent versioning strategy. This flow: * Generates a "release specification" file containing all of the workspace packages within the repo (this will be changed to all _updated_ packages in a future commit) * Opens the user's editor (if one can is detected) and waits for them to edit it * Parses the edited version of the release spec * Applies the release spec by: * Updating the version of the root package with the current date (note: the format of the version string will be changed in a future commit) * Updating the versions of all of the packages listed in the release spec * Adds new sections to the changelogs of all of the packages listed in the release spec

mcmire

Okay, here's the PR I've been working on for the past couple of weeks. I know this is a giant PR 😓 . I really tried to pare this branch down as much as I could, but in addition to introducing a flow this also adds a bunch of infrastructure as well which would not be used without that flow.

I'm happy to walk through this code on a call, but if you want to brave a readthrough, I would recommend skipping tests and reading the files in this order (all of these files are in src/):

index.ts
main.ts
initialization-utils.ts
inputs-utils.ts
project-utils.ts
git-utils.ts
package-utils.ts
package-manifest-utils.ts
monorepo-workflow-utils.ts
editor-utils.ts
release-specification-utils.ts
workflow-utils.ts

~~Also, there are a lot of TODOs in the code. I plan on extracting those into another PR as I want to be able to link to places in the code.~~ Done.

mcmire · 2022-07-18T22:48:52Z

jest.config.js

+  coveragePathIgnorePatterns: [
+    '/node_modules/',
+    '/src/index.ts',
+    '/src/inputs-utils.ts',


This file uses yargs to parse command-line arguments. It's pretty hard to mock yargs given its flexible API, and this is the only thing this file does, so I opted not to write tests for it. If there's a way to do this however then I'd love to know!

src/editor-utils.ts

mcmire · 2022-07-18T22:55:44Z

src/git-utils.ts

+ * @returns The HTTPS URL of the repository, e.g.
+ * `https://github.com/OrganizationName/RepositoryName`.
+ */
+export async function getRepositoryHttpsUrl(


Some of these functions were lifted directly from action-create-release-pr, such as this one.

Huh, we could use this function in @metamask/auto-changelog. Right now that only supports HTTPS repository URLs.

Are you talking about replacing this function. If so, that pulls the URL from package.json, whereas this one pulls the URL from Git, so that's also a difference.

Right, good point, I was just thinking of the second half of this function that performs normalization.

src/monorepo-workflow-utils.test.ts

mcmire · 2022-07-18T23:10:02Z

src/package-manifest-utils.ts

@@ -0,0 +1,348 @@
+import path from 'path';


A lot of this file was stolen from package-utils.ts in action-utils and consolidated/simplified.

mcmire · 2022-07-18T23:13:14Z

src/release-specification-utils.ts

@@ -0,0 +1,298 @@
+import fs, { WriteStream } from 'fs';


This is all new stuff.

mcmire · 2022-07-18T23:13:29Z

src/workflow-utils.ts

@@ -0,0 +1,74 @@
+import { Package } from './package-utils';


This is new stuff.

mcmire · 2022-07-18T23:14:36Z

src/package-utils.ts

+ * @param args.stderr - A stream that can be used to write to standard error.
+ * @returns The result of writing to the changelog.
+ */
+async function updatePackageChangelog({


updatePackageChangelog and updatePackage were more or less stolen from package-operations in action-create-release-pr.

mcmire · 2022-07-18T23:15:01Z

src/monorepo-workflow-utils.ts

@@ -0,0 +1,244 @@
+import type { WriteStream } from 'fs';


This is new stuff too.

src/file-utils.ts

mcmire · 2022-07-18T23:18:05Z

Also I know that there are linting errors, I will address those tomorrow 😓

jest.config.js

tsconfig.json

.eslintrc.js

mcmire · 2022-07-19T22:49:22Z

src/package-manifest-utils.ts

+        validation: validationForManifestDependenciesField,
+        defaultValue: {},
+      });
+      return { ...obj, [fieldName]: dependencies };


This should not include fields that are not in the original manifest. This means that when we write back to the manifest in updatePackage we are adding unnecessary data. I'll make a note to fix this in a future PR.

mcmire · 2022-07-19T22:50:00Z

src/package-manifest-utils.ts

+    [ManifestFieldNames.Version]: version,
+    [ManifestFieldNames.Workspaces]: workspaces,
+    [ManifestFieldNames.Private]: privateValue,
+    ...dependencyFields,


This function looks for fields in the manifest that we care about, but completely discards all fields that we don't care about. This isn't right — this means when we write back to the manifest in updatePackage we are removing critical fields. I've made a note to fix this in a future PR.

jest.config.js

Gudahtt

I really like how this is organized! I will try to finish reading through the non-test code tomorrow morning, and hopefully will start on the tests as well.

src/index.ts

src/initialization-utils.ts

src/misc-utils.ts

src/inputs-utils.ts

src/project-utils.test.ts

src/git-utils.test.ts

Gudahtt · 2022-07-27T17:14:53Z

src/misc-utils.ts

+ * returns that `message` by default.
+ * @returns A new error object.
+ */
+export function wrapError(


Have you seen this proposal before? https://github.com/tc39/proposal-error-cause
It's now stable, and was shipped in Node.js v16.9 and in all major browsers, and has a shim. It's a standard way of "wrapping" an error, and I think it would be better to use this pattern than by decomposing and reconstructing an error like is done here.

I haven't! This is cool, but does Node v14 (or v16.9) use the cause property when printing out an error? In other words, if cause points to another Error object that has a stack, I'd want that error to get automatically printed along with the new error — kind of like this, but without having to use an explicit function to get that stack. I think that'd be my only concern.

Looks like that's how it looks in practice:

$ node Welcome to Node.js v16.14.2. Type ".help" for more information. > const innerError = new Error('inner') undefined > const outerError = new Error('outer', { cause: innerError }) undefined > console.error(outerError) Error: outer at REPL2:1:20 at Script.runInThisContext (node:vm:129:12) ... 7 lines matching cause stack trace ... at REPLServer.Interface._line (node:readline:864:8) { [cause]: Error: inner at REPL1:1:20 at Script.runInThisContext (node:vm:129:12) at REPLServer.defaultEval (node:repl:566:29) at bound (node:domain:421:15) at REPLServer.runBound [as eval] (node:domain:432:12) at REPLServer.onLine (node:repl:893:10) at REPLServer.emit (node:events:538:35) at REPLServer.emit (node:domain:475:12) at REPLServer.Interface._onLine (node:readline:487:10) at REPLServer.Interface._line (node:readline:864:8) } undefined >

Hmm... that would work for Node v16, but I'm not sure it would work for v14, even when using a polyfill, because v8 doesn't know to look for cause when producing a stack trace. This would particularly impact our fs.promises wrappers because it means that when an error is output, the stack we want to add in the first place would get lost. But this would also impact every place we're using wrapError in order to simply prefix existing error messages, because the original error message would get lost too. So I'm not sure that this is something that would help us until we switched everything to v16.

Thoughts?

This would particularly impact our fs.promises wrappers because it means that when an error is output, the stack we want to add in the first place would get lost.

Hmm, would it? I thought the stack that we cared about was the outer error stack, because the inner one had no stack.

But yes that's a fair point overall, it looks like this would only show the outer error on Node.js v14. We could account for that by incorporating the inner error message in the outer message. Or by using pony-cause utilities instead of just console.error(error) directly. Or accepting that using this in v14 would result in degraded error messages (we can set .npmrc to v16.x in all of our libraries and still keep the minimum set to v14).

I am not sure about there being a real need to recursively search through the cause stack to find a code.

Is there a case where we don't know where to find the code we're looking for? Like in this case, even if we keep the code on the wrapped error, we know it will be on error.cause.code. If we don't know where the error with the code is, why would we check for it? Checking recursively for any cause risks mistaking one error for another.

Another thing to consider here is that we can make these fs-wrapper errors be whatever we like. We could forward the code along to the outer error, or we could use error classes to distinguish cases.

Is there a case where we don't know where to find the code we're looking for? Like in this case, even if we keep the code on the wrapped error, we know it will be on error.cause.code.

Sure, take this example in package.test.ts, where we're reading the changelog file. You and I know what readFile is doing, but I don't want anyone else to have to know that. They should just be able to grab the code from the error object as if it's not wrapped.

Since that's the goal, and since I don't want to add too much magic to our fs wrappers here so they can be used in a flexible way, I'll just copy code over.

Okay, see 761377f. I opted to use pony-cause instead of error-cause as I didn't want to change any globals.

You and I know what readFile is doing, but I don't want anyone else to have to know that. They should just be able to grab the code from the error object as if it's not wrapped.

I don't think that scenario matches up. I was asking for an example where the user wants to find the code, but doesn't know how many layers deep it is. Not an example where the user might wrongly expect it to be at the top-level. In this example you gave, why would they use this "recursively find code" function as opposed to directly accessing the code?

That's what I was getting at - that if the user expects a code at all, it's likely because they are checking for a specific error condition which would have the code in a known location.

It sounds like you're saying users might get tripped up here by confusing our readFile with fs.promises.readFile. And that by making ours behave more like fs.promises.readFile we'd avoid confusion. Totally agreed there, that makes perfect sense.

I opted to use pony-cause instead of error-cause as I didn't want to change any globals.

That makes sense. Though we could get away with using neither. In Node.js v14, the cause property doesn't cause the Error constructor to blow up or anything. It just gets ignored. Not ideal of course, but considering that we don't expect this to get used on Node.js v14 in practice, maybe we don't care? The wrapper error with the stack probably provides enough context in most cases anyway, especially if the code is being set on the wrapper error.

Using pony-cause has advantages though. It avoids a potential foot-gun if we ever have runtime checks for cause. And we might want to use a similar pattern in libraries that will be run with Node.js v14.

src/monorepo-workflow-utils.ts

src/env-utils.test.ts

src/misc-utils.ts

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

package.json

src/file-utils.test.ts

src/command-line-arguments.ts

tests/unit/helpers.ts

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

src/package.ts

src/package-manifest.ts

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

src/package-manifest.ts

src/monorepo-workflow-operations.ts

src/monorepo-workflow-operations.test.ts

src/monorepo-workflow-operations.ts

Gudahtt

LGTM!

There is one outstanding discussion regarding wrapError, but that doesn't need to block.The PR as a whole is in fantastic shape.

I didn't review the workflow operations tests line-by-line, just skimmed to get a general sense. I'll look at those more closely in the follow-up PRs that affect those I'm sure.

Gudahtt · 2022-08-02T21:24:24Z

src/misc-utils.ts

+    return error;
+  }
+
+  return new Error(`${message}: ${originalError}`);


Nit: imo it is printed more nicely if ErrorWithCause is used, even for non-error-types. cause doesn't have to be an error type. It would simplify this function to remove that case as well.

So, it seems that the type for ErrorWithCause is wrong in pony-cause, although to be fair, this is because it's trying to be compatible with TypeScript and the type for Error is wrong there too. This was fixed here and it looks like it'll be available in TypeScript 4.8. I could patch pony-cause, but I think that would only apply to development — the types would be broken in production. So I think we should think about this a little bit more.

That issue seems to indicate that updating TypeScript would fix the issue. We can do that later.

Gudahtt · 2022-08-02T21:26:06Z

src/misc-utils.ts

+ * something throwable).
+ * @returns A new error object.
+ */
+export function coverError(message: string, originalError: unknown) {


Nit: why the change in name? wrapError seemed appropriate, and is what I've always heard this called. The spec uses the verb "wrap" for this pattern as well.

Though I guess in the general case of wrapping errors, we can use ErrorWithCause directly, and there would be no need for this function to exist. Except that it passes along the inner code. Was the name change related to the code behaviour?

No problem. Changed in b4d820d.

mcmire · 2022-08-02T22:05:29Z

Okay one more :)

mcmire added 2 commits July 18, 2022 16:18

Use Babel for Jest coverage provider

3d5b014

v8 isn't quite reliable when using Node 14 for development (it has too many false negatives). We can consider changing this back if/when we switch to Node 16 or later, but for now we will stick to Babel.

mcmire requested a review from a team as a code owner July 18, 2022 22:48

mcmire commented Jul 18, 2022

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

jest.config.js Outdated Show resolved Hide resolved

mcmire commented Jul 19, 2022

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

mcmire added 3 commits July 19, 2022 09:21

Fix lint issues / roll back some changes

10d62bc

Fix test failure on Node 16

cc69588

Re-enable this test

557b5c1

Base automatically changed from use-babel-for-coverage to main July 19, 2022 16:25

mcmire commented Jul 19, 2022

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

Clean up a couple of things

9b2d02f

mcmire commented Jul 19, 2022

View reviewed changes

Gudahtt reviewed Jul 20, 2022

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

mcmire mentioned this pull request Jul 20, 2022

Don't blow away package manifest when updating #20

Merged

Remove TODOs

c622b3a

mcmire mentioned this pull request Jul 22, 2022

Simplify monorepo-workflow-operations #21

Merged

mcmire self-assigned this Jul 25, 2022

Gudahtt reviewed Jul 26, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Jul 27, 2022

View reviewed changes

src/monorepo-workflow-utils.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Jul 27, 2022

View reviewed changes

src/env-utils.test.ts Outdated Show resolved Hide resolved

src/misc-utils.ts Outdated Show resolved Hide resolved

src/misc-utils.ts Show resolved Hide resolved

mcmire and others added 3 commits July 27, 2022 12:16

Fix name of test in git-utils

f7c1538

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Merge branch 'main' into add-first-flow

3dfa8b0

Respond to feedback

137c0d9

Gudahtt reviewed Jul 27, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

mcmire added 2 commits July 27, 2022 12:43

Fix docs for resolveExecutable

29545e5

Remove deepmerge

7a9af7e

Gudahtt reviewed Jul 27, 2022

View reviewed changes

src/file-utils.test.ts Outdated Show resolved Hide resolved