feat(tools-packages): Add package validation helpers to tools-packages

[JasonVMo]

Description

I started this code while doing some package linting work in the FURN repository where I wanted to shift some of the work to yarn constraints. It had what I thought was a fairly good pattern for enforcing rules on package.json files and realized this could be useful as general helpers for authoring rules against package.json files (and other JSON files).

This has a few main parts.

PackageContext

This is a raw bundle of readonly manifest, name, and root path. It is essentially the core of the PackageInfo type but the creation functions for it are uncached, whereas PackageInfo are cached and contain information about whether or not the package is a workspace.

JSONValidator

This can be created in fix mode or in check mode. By use of the enforce routine the manifest will be updated (if in fix) or emit errors (if in check). When the finish routine is called the manifest will be written out if there are changes. This can be created against any JSON file and is not package.json specific.

PackageValidationContext

This a a validating context which can be created against a folder/manifest in the repo, or from a Yarn.Constraints.Workspace allowing validation rules to be written such that they can run in yarn constraints calls or in other contexts. The dependency on @yarnpkg/types where this type comes from is an optional peer dependency as it only needs to be filled in when you are using constraints where you should already be typing your constraints via that package.

Test plan

Added automated tests for the new functionality.

[tido64]

I'm not seeing the benefits of this when Yarn already solves the exact same problem that we've already implemented in rnx-kit.

rnx-kit/yarn.config.cjs

Lines 16 to 43 in 18bf2c4

	for (const workspace of Yarn.workspaces()) {
	const { name, private: isPrivate, experimental } = workspace.manifest;
	if (isPrivate && !experimental) {
	workspace.set("version", "0.0.0");
	}
	workspace.set("author.name", author.name);
	workspace.set("author.email", author.email);
	const homepage =
	workspace === root
	? `${origin.url}#readme`
	: `${origin.url}/tree/main/${workspace.cwd}#readme`;
	workspace.set("homepage", homepage);
	workspace.set("repository.type", origin.type);
	workspace.set("repository.url", origin.url);
	if (workspace !== root) {
	workspace.set("repository.directory", workspace.cwd);
	}
	if (name.startsWith("@rnx-kit/yarn-plugin-")) {
	const engines = getRootEnginesField();
	workspace.set("engines.node", engines.node);
	workspace.set("engines.yarn", ">=4.0");
	}
	}

I think it would make more sense if the intention is to bring Yarn Constraints to other package managers or into other workloads, and that the syntax is basically the same.

[JasonVMo]

There are two questions here. The usefulness and the API. In terms of usefulness, yarn constraints are limited in applicability. There are a number of places where you want to run package validation logic, one of which is yarn constraints. This pattern allows you to author that code once and then have it be usable in multiple contexts. Align-deps is an example. A follow up PR here would be to have align-deps use this structure for package.json modifications, thus allowing it to be run via API in constraints if desired. Another point of the usefulness is that the yarn constraints model only works for package.json files. In FURN I use this same pattern for tsconfig.json validation as well.

The other question is API surface. The set/unset/error pattern is pretty clean, but I'm less a fan of using set as it is an extremely generic name and has different semantics than what actually happens. I chose the undefined to delete mainly because it allows better generic patterns (like loop handling) rather than having to branch the logic between deletes and adds.

[tido64]

That makes sense to me. It does sound like we want a new JSON validation package though. I don't think it fits in tools-package if it's supposed to be able to handle any .json file, not just package.json, which is the main purpose of this package.

[tido64]

Can we use Record<symbol, T> instead of object here to avoid casting? I've managed to reduce it to 1 casting with this:

   friendlyName: string,
   initialize: (pkgInfo: PackageInfo) => T
 ): GetPackageValue<T> {
+  return createValueLoader<T, PackageInfo<T>>(friendlyName, initialize);
+}
+
+/**
+ * Helper function to create a typed accessor function for getting and storing information
+ * in any object. This can be whatever you want, the key is only created and stored in
+ * the generated function so there are no collisions.
+ *
+ * @param friendlyName name used to create a symbol key for the package info
+ * @param initialize function used to initialize the value stored in the key
+ * @returns a function to retrieve the value from the object, if unset the initialize function is called
+ */
+export function createValueLoader<T, TObj extends Record<symbol, T>>(
+  friendlyName: string,
+  initialize: (obj: TObj) => T
+): (obj: TObj) => T {
   const symbolKey = Symbol(friendlyName);
-  return (pkgInfo: PackageInfo) => {
-    if (!(symbolKey in pkgInfo)) {
-      pkgInfo[symbolKey] = initialize(pkgInfo);
+  return (obj: TObj) => {
+    if (!Object.hasOwn(obj, symbolKey)) {
+      (obj as Record<symbol, T>)[symbolKey] = initialize(obj);
     }
-    return pkgInfo[symbolKey] as T;
+    return obj[symbolKey];
   };
 }

And the following changes in types.ts:

diff --git a/packages/tools-packages/src/types.ts b/packages/tools-packages/src/types.ts
index cbcdb16cf..eb67240fd 100644
--- a/packages/tools-packages/src/types.ts
+++ b/packages/tools-packages/src/types.ts
@@ -1,6 +1,6 @@
 import type { PackageManifest } from "@rnx-kit/types-node";

-export type PackageInfo = {
+export type PackageInfo<T = unknown> = {
   /** name of the package */
   name: string;

@@ -18,13 +18,13 @@ export type PackageInfo = {
    * other packages to leverage any caching happening for PackageInfo entries to store additional
    * information and ensure it is only loaded once.
    */
-  [key: symbol]: unknown;
+  [key: symbol]: T;
 };

 /**
  * Typed accessors for retrieving values from the package info
  */
-export type GetPackageValue<T> = (pkgInfo: PackageInfo) => T;
+export type GetPackageValue<T> = (pkgInfo: PackageInfo<T>) => T;

 /**
  * Set of accessor functions that can be retrieved for a specific symbol

The final cast can't be removed because of TS2862 Type 'T' is generic and can only be indexed for reading. Which I guess is a limitation in TypeScript itself.

[tido64]

We don't have to use unknown here. This is what we use in RNTA:

export type JSONValue =
  | string
  | number
  | boolean
  | JSONArray
  | JSONObject
  | null;

export type JSONArray = JSONValue[];
export type JSONObject = { [key: string | symbol]: JSONValue };

Note that we don't use Record because TypeScript doesn't like circular references.

[tido64]

This (along with the validation code) doesn't fit in tools-package. Consider moving to a separate package (not necessarily with a tools- prefix).

[tido64]

Suggested change

	jsonFilePath: jsonFilePath,
	jsonFilePath,

[tido64]

Can this be configured to delete/ignore instead of throwing similar to Node's --disable-proto?

[tido64]

It looks like we already handle fix being unset so we can make this smaller:

Suggested change

	const defaultOptions: Omit<JSONValidatorOptions, "jsonFilePath"> = {
	fix: false,
	};
	const defaultOptions: Omit<JSONValidatorOptions, "jsonFilePath"> = {};