Fix package manifest validation and typing #25

rekmarks · 2021-05-12T05:00:57Z

This PR improves the validation and type checking of package manifest objects. Previously, we were casting package manifest objects and using Partial needlessly. Now, the validatePackageManifest function acts as a type guard by returning the unmodified package manifest object, but with correct types.

As part of this improved validation, readJsonFile has been renamed to readJsonObjectFile and rewritten to error if the parsed JSON is anything other than a plain object, as opposed to just a falsy value.

src/utils.test.ts

src/package-operations.ts

Gudahtt · 2021-05-13T01:09:31Z

src/package-operations.ts

-export function validatePackageManifest(
-  manifest: Partial<PackageManifest>,
+export function validatePackageManifest<
+  T extends Partial<PackageManifest>,


I remain a bit confused about why these generic parameters are necessary. It seems that there are two types of manifests that we want to validate: a monorepo manifest, and a non-monorepo manifest. Why do we need two generic parameters, and to pass in which fields to validate? This all seems a bit... overly complicated? Maybe I'm missing something

It was complicated by the fact that there's effectively three different types of package manifests, going from subset to superset.

The first is the root manifest, which could be either a normal or a monorepo package.json file. There, we only need to validate the version. For the polyrepo case, we also need to validate name. For the monorepo case, we need to also validate private and workspaces. On top of that, deeper in the monorepo workflow, we need to validate version and name for the manifests of the monorepo's packages.

Rather than keeping a single generic function around, I split the validation logic out of getPackageManifest and created separate validation function for all the different cases. Most of these new functions use a single generic, but it's a convenient to take in e.g. a Partial manifest object and return it with e.g. one field correctly type.

I think this is probably better, because the generic validation function kind of sucked.

Part of what I was missing is that you were aiming to perform the absolute minimum validation necessary. I would have probably used just a single validation function that validated name and version, and also conditionally validated workspaces and private if they existed (and that private was true if workspaces was present). That'd give us one PackageManifest type. Then a simple check that workspaces existed would give us a second type, MonorepoPackageManifest.

But skipping the name validation for the root does make that a bit more challenging.

But skipping the name validation for the root does make that a bit more challenging.

Yeah, validating the name would require that we mandate that monorepo root manifests have a placeholder name field, which seems kind of bad.

Gudahtt · 2021-05-13T13:29:16Z

src/package-operations.ts

@@ -313,101 +320,132 @@ function getUpdatedDependencyField(
 * @param fieldsToValidate - The manifest fields that will be validated.


This description requires updating, now that the validation isn't being done here.

Gudahtt · 2021-05-13T13:31:19Z

src/package-operations.ts

-export function validatePackageManifest(
-  manifest: Partial<PackageManifest>,
+export function validatePackageManifestVersion<
+  T extends Partial<PackageManifest>


Nit: A JSDoc description of this would be helpful

Same goes for the other generics introduced in this PR

Gudahtt · 2021-05-13T13:33:52Z

src/package-operations.ts

@@ -31,9 +31,12 @@ export enum FieldNames {
 export interface PackageManifest
  extends Partial<Record<PackageDependencyFields, Record<string, string>>> {
  readonly [FieldNames.Name]: string;
-  readonly [FieldNames.Private]?: boolean;


Why remove these? These are valid properties of a package manifest. Removing these forces you to use MonorepoPackageManifest for any situation where you might receive either type of manifest (e.g. in getManifestErrorMessagePrefix), which is semantically confusing because it wrongly implies that a non-monorepo manifest isn't expected.

Gudahtt · 2021-05-13T13:47:11Z

src/package-operations.ts

@@ -31,9 +31,12 @@ export enum FieldNames {
 export interface PackageManifest
  extends Partial<Record<PackageDependencyFields, Record<string, string>>> {
  readonly [FieldNames.Name]: string;


MonorepoPackageManifest is used without validating the name, yet the type declares that the name exists. This mistake is missed by TypeScript because of the as MonorepoPackageManifest cast.

If you really want to skip validating the name, this should be made optional.

Gudahtt · 2021-05-13T13:56:43Z

src/package-operations.ts

+      }`,
+    );
+  }
+  return manifest as MonorepoPackageManifest;


This cast can be avoided by using type guards for the conditions above.

Gudahtt · 2021-05-13T14:01:26Z

src/update.ts

@@ -110,7 +119,7 @@ async function updateMonorepo(
  newVersion: string,
  versionDiff: SemverReleaseType,
  rootManifest: Pick<


Could this use the MonorepoPackageManifest type directly instead, rather than picking just those two properties?

rekmarks · 2021-05-13T21:25:51Z

I think I've addressed all outstanding comments in 51be3ed. The type guards are more verbose, but we avoid casting using as, and I think end up in a safer place. I also fixed the various package manifest types.

Gudahtt

LGTM!

rekmarks force-pushed the manifest-validation branch from e7a03a7 to 3213774 Compare May 12, 2021 05:08

rekmarks mentioned this pull request May 12, 2021

Add changelog updating #23

Merged

Base automatically changed from changelog-updating to main May 12, 2021 19:31

rekmarks added 2 commits May 12, 2021 12:33

Fix package manifest typing

aceedd7

Fix readJsonObjectFile test

86ec0ec

rekmarks force-pushed the manifest-validation branch from 3213774 to 86ec0ec Compare May 12, 2021 19:35

rekmarks requested a review from Gudahtt May 12, 2021 19:36

rekmarks marked this pull request as ready for review May 12, 2021 19:36

Gudahtt reviewed May 13, 2021

View reviewed changes

Split validation logic into multiple functions

b46666d

Gudahtt reviewed May 13, 2021

View reviewed changes

Use type guards, fix types

51be3ed

Gudahtt approved these changes May 13, 2021

View reviewed changes

rekmarks merged commit 289b086 into main May 13, 2021

rekmarks deleted the manifest-validation branch May 13, 2021 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix package manifest validation and typing #25

Fix package manifest validation and typing #25

rekmarks commented May 12, 2021 •

edited

Loading

Gudahtt May 13, 2021

rekmarks May 13, 2021

Gudahtt May 13, 2021

rekmarks May 13, 2021

Gudahtt May 13, 2021

Gudahtt May 13, 2021

Gudahtt May 13, 2021

Gudahtt May 13, 2021

Gudahtt May 13, 2021

Gudahtt May 13, 2021

Gudahtt May 13, 2021

rekmarks commented May 13, 2021

Gudahtt left a comment

		@@ -313,101 +320,132 @@ function getUpdatedDependencyField(
		* @param fieldsToValidate - The manifest fields that will be validated.

Fix package manifest validation and typing #25

Fix package manifest validation and typing #25

Conversation

rekmarks commented May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rekmarks commented May 13, 2021

Gudahtt left a comment

Choose a reason for hiding this comment

rekmarks commented May 12, 2021 •

edited

Loading