refactor(dep-check): introduce the new configuration schema #1911

tido64 · 2022-09-30T06:47:39Z

Description

Introduces the new config schema. In particular, the old -> new config migration is implemented along with the check command. This can be flipped on with an env variable.

Test plan

Build align-deps:

cd packages/align-deps
yarn build --dependencies
cd ../test-app

Change the test app's target prod version to 0.70:

diff --git a/packages/test-app/package.json b/packages/test-app/package.json
index 43107f04..3a138093 100644
--- a/packages/test-app/package.json
+++ b/packages/test-app/package.json
@@ -61,7 +61,7 @@
     "preset": "@rnx-kit/scripts"
   },
   "rnx-kit": {
-    "reactNativeVersion": "^0.68",
+    "reactNativeVersion": "^0.70",
     "kitType": "app",
     "build": {
       "distribution": [

Run align-deps:

node ../align-deps/lib/cli.js

Follow the instructions on screen, and repeat the command. The warnings should go away.

packages/dep-check/src/check.ts

afoxman

It's exciting to see align-deps coming along! Sorry this took me so long to get to.

I left comments throughout the code, and had a few PR-wide comments as well:

The package is getting complex, and so the individual functions and code segments are going to be harder to "parse" and understand.

One thing that would help is comments, both at the function level and in lengthy blocks that are doing key data transformations.

Another is function names that express the data query or transform that is happening. For example, v2_profilesSatisfying. As an outsider who isn't immersed in dependency management/alignment challenges, I'm not sure what this name is telling me. Also, in this case, the function operates on requirements and presets, not profiles. So maybe "v2_filterAndMergePresetsSatisfyingRequirements". And this name is ugly, and leads me to think there are probably two or three functions it should split into.

Old vs New config. It seems like there's a straightforward, automatic way to internally transform from old to new. Internally, I see hints of the old config here and there. I'm wondering if you can localize these, pushing up to the CLI/API entry point? When an old config comes in, do a single transform, and then the other 95% of align-deps operates only on the new config. I realize this muddies error-reporting. e.g. telling users about 'preset X' and 'requirement R' in a message could confuse them if they're on the old config. They may still have enough info to puzzle it out, though, and you could append a link to new config in the error message if they want to learn more. There is value in both customer happiness and tool usability.

packages/config/src/kitConfig.ts

packages/dep-check/src/check.ts

packages/dep-check/src/compatibility/config.ts

packages/dep-check/src/dependencies.ts

packages/dep-check/src/presets/microsoft/react-native.ts

packages/dep-check/src/profiles.ts

tido64

I should have made this clearer in the description. The scope of this PR is limited to the new config schema, and the check command. As such, dep-check will be in a state of messy mash of both v1 and v2 until we've migrated everything.

Currently, the plan is to:

Migrate commands to the new config schema
Copy all code to a new package, align-deps
Remove all v1 code
Publish align-deps
Have dep-check wrap align-deps with a warning about the name change

But if this is confusing, maybe it makes more sense to swap 1 and 2 here. What do you think?

packages/dep-check/src/check.ts

tido64 · 2022-10-10T08:23:30Z

packages/dep-check/src/check.ts

-    return checkPackageManifest(manifest, options);
-  };
+  const check =
+    process.env["DEP_CHECK_VERSION"] === "2"


I don't expect DEP_CHECK_VERSION to be long lived. It only exists now while I implement v2. Mostly for making testing easier. It's not meant to be public.

tido64 · 2022-10-10T08:23:37Z

packages/dep-check/src/compatibility/config.ts

+        ...(customProfilesPath ? [customProfilesPath] : []),
+      ],
+      requirements:
+        kitType === "app"


This config is also stringified and printed to the user further down. Added a comment here.

packages/dep-check/src/compatibility/config.ts

packages/dep-check/src/dependencies.ts

packages/dep-check/src/presets/microsoft/react-native.ts

packages/dep-check/src/profiles.ts

tido64 · 2022-10-10T11:57:06Z

I will rebase this PR when #1927 lands.

kelset · 2022-10-11T11:55:49Z

packages/align-deps/src/check.ts

+
+  const devPreset = (() => {
+    if (kitType === "app") {
+      // Preset for development is unused when the package is an app.


sidenote: we need to make sure this is clear in the docs - I can see people not be aware

packages/align-deps/src/presets/microsoft/react-native.ts

packages/test-app/package.json

kelset

overall, given that the logic changes are already happening in the new package, I think we can go ahead and merge this.

Thanks for all your work Tommy, really appreciate that you keep the testing side up to date and enhanced with more tests.

I've left a few comments but they are all things not strictly related to the code

afoxman

Seeing align-deps separate from dep-check makes things easier to understand. I have some new questions from this round of feedback, and a few unresolved comments from the last round.

I'll leave it to you to decide how and when to submit this PR and track the open items. I don't want to hold this up, but I would like those items addressed at some point.

If you submit this PR as-is, can you mark the align-deps version as 0.x or 0.0.x and use an '-alpha' prerelease tag for now?

afoxman · 2022-10-12T02:37:08Z

packages/align-deps/src/check.ts

+}
+
+function resolve(
+  { kitType, alignDeps, manifest }: AlignDepsConfig,


Suggested change

{ kitType, alignDeps, manifest }: AlignDepsConfig,

{ kitType, alignDeps: { capabilities, presets, requirements }, manifest }: AlignDepsConfig,

Can you do nested destructuring like this? More of a curiosity than a suggestion...

Yes, you can. It affects legibility so I don't do it as much.

afoxman · 2022-10-12T02:47:25Z

packages/align-deps/src/cli.ts

 }

 if (require.main === module) {
  require("yargs").usage(
    "$0 [packages...]",
-    "Dependency checker for React Native apps",
+    "Dependency checker for npm packages",


Is "Dependency checker" still the friendly name you want to use for align-deps?

Do you have any suggestions? This package is still private so there's still time to change it. I'll make it part of the docs update PR.

afoxman · 2022-10-12T02:50:08Z

packages/align-deps/src/compatibility/config.ts

-    .map((v) => v.trim().split(".").slice(0, 2).join("."))
+    .map((v) => {
+      const coerced = semverCoerce(v);
+      return coerced ? `${coerced.major}.${coerced.minor}` : "0.0";


Are there times when this would realistically fail? And would you want to return a fake value when that happens, or fail the current manifest/run?

I'm guessing this happens if someone types something wrong in a preset file or in package.json. Seems like a good reason to just stop since you no longer can trust what you are looking at.

This function was moved in more recent changes. I'll make this throw in a later PR.

afoxman · 2022-10-12T02:57:22Z

packages/align-deps/src/dependencies.ts

+    }
+
+    const filteredPreset = filterPreset(requirements, preset);
+    const filteredNames = Object.keys(filteredPreset);


Are the resulting filteredNames all packages which didn't meet the requirements in the preset? e.g. the stuff that is out of alignment?

I've added more comments around this in a separate PR, but what happens here is that we filter out profiles that do not fulfill the requirements. So filteredNames is the list of profile names that do satisfy the requirements.

afoxman · 2022-10-13T01:58:35Z

packages/align-deps/src/dependencies.ts

+          : requirements.production;
+      }
+
+      if (kitConfig.reactNativeVersion) {


Should this be scoped to an "if old config is being used" check? Or will it be in the future?

Or maybe since this PR is now part of align-deps, will you add some deprecation warnings when you run into and make use of config like this?

This for detecting old config in third-parties. I don't know if it makes sense to output warnings here as it may be too noisy.

afoxman · 2022-10-13T02:07:55Z

packages/align-deps/src/preset.ts

+        pkg.name === requiredPackage &&
+        "version" in pkg &&
+        semverSatisfies(
+          semverCoerce(pkg.version) || "0.0.1",


Is there an example of when semverCoerce would legitimately fail, or is this the "0.0.1" an unlikely fallback? I'm not sure align-deps should try to run if it encounters a package with an invalid manifest. I think it's reasonable to fail and bail, rather than risk operating on other potentially bad data.

I think this would fail if you fed it random strings. I don't expect this to fail often, but I've changed it to throw instead. I will also change the other sites, but in separate PRs since I've moved stuff around a bit.

tido64 · 2022-10-13T09:16:18Z

@afoxman, @kelset: Thanks for reviewing! I'm going to merge this now and will try to address all your concerns in upcoming PRs. I have five lined up already and they all depend on one another.

tido64 requested review from afoxman, kelset and JasonVMo as code owners September 30, 2022 06:47

tido64 mentioned this pull request Sep 30, 2022

RFC 0001: dep-check v2 #1890

Closed

24 tasks

tido64 commented Sep 30, 2022

View reviewed changes

packages/dep-check/src/check.ts Outdated Show resolved Hide resolved

tido64 force-pushed the tido/dep-check-v2 branch 2 times, most recently from 610adaa to b910879 Compare October 6, 2022 08:31

afoxman reviewed Oct 7, 2022

View reviewed changes

tido64 commented Oct 10, 2022

View reviewed changes

tido64 added 8 commits October 10, 2022 14:31

refactor(align-deps): introduce the new configuration schema

488abdf

update update-readme script

a5c9680

error handling

92381c2

address feedback

24dbb8b

split v2_profilesSatisfying

742f29f

move preset functions to separate file

2d97a88

move changes to align-deps

b60928d

clean up v1 code and add tests

2c5ad0a

tido64 force-pushed the tido/dep-check-v2 branch from 4888388 to 2c5ad0a Compare October 10, 2022 12:53

refactor cli

c776457

tido64 force-pushed the tido/dep-check-v2 branch from c2c3a2a to c776457 Compare October 10, 2022 18:05

kelset reviewed Oct 11, 2022

View reviewed changes

packages/align-deps/src/presets/microsoft/react-native.ts Show resolved Hide resolved

kelset reviewed Oct 11, 2022

View reviewed changes

packages/test-app/package.json Show resolved Hide resolved

kelset approved these changes Oct 11, 2022

View reviewed changes

afoxman approved these changes Oct 13, 2022

View reviewed changes

throw when semverCoerce fails

b3ec7ef

tido64 merged commit e1d4b24 into microsoft:main Oct 13, 2022

tido64 deleted the tido/dep-check-v2 branch October 13, 2022 09:16

tido64 added the feature: align-deps This is related to align-deps label Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(dep-check): introduce the new configuration schema #1911

refactor(dep-check): introduce the new configuration schema #1911

tido64 commented Sep 30, 2022 •

edited

Loading

afoxman left a comment

tido64 left a comment

tido64 Oct 10, 2022

tido64 Oct 10, 2022

tido64 commented Oct 10, 2022

kelset Oct 11, 2022

kelset left a comment

afoxman left a comment

afoxman Oct 12, 2022

tido64 Oct 13, 2022

afoxman Oct 12, 2022

tido64 Oct 13, 2022

afoxman Oct 12, 2022

tido64 Oct 13, 2022

afoxman Oct 12, 2022

tido64 Oct 13, 2022

afoxman Oct 13, 2022

afoxman Oct 13, 2022

tido64 Oct 13, 2022

afoxman Oct 13, 2022

tido64 Oct 13, 2022

tido64 commented Oct 13, 2022 •

edited

Loading

	{ kitType, alignDeps, manifest }: AlignDepsConfig,
	{ kitType, alignDeps: { capabilities, presets, requirements }, manifest }: AlignDepsConfig,

refactor(dep-check): introduce the new configuration schema #1911

refactor(dep-check): introduce the new configuration schema #1911

Conversation

tido64 commented Sep 30, 2022 • edited Loading

Description

Test plan

afoxman left a comment

Choose a reason for hiding this comment

tido64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tido64 commented Oct 10, 2022

Choose a reason for hiding this comment

kelset left a comment

Choose a reason for hiding this comment

afoxman left a 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

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

tido64 commented Oct 13, 2022 • edited Loading

tido64 commented Sep 30, 2022 •

edited

Loading

tido64 commented Oct 13, 2022 •

edited

Loading