Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

align-deps: new flag to not throw an error for exact dependencies that are within bounds #2983

Open
balazsgerlei opened this issue Feb 21, 2024 · 6 comments · May be fixed by #2984
Open

align-deps: new flag to not throw an error for exact dependencies that are within bounds #2983

balazsgerlei opened this issue Feb 21, 2024 · 6 comments · May be fixed by #2984
Labels
enhancement New feature or request feature: align-deps This is related to align-deps needs attention Extra attention is needed

Comments

@balazsgerlei
Copy link
Contributor

Both myself and the company I work for started using @rnx-kit/align-deps and it would be great to add it to CI as well, potentially even blocking PR merge if it throws an error. However we use exact versions for certain dependencies, which even if are within the "bounds" of what align-deps checks for results in an error for not using the exact sytnax for versioning.

While I understand that the current behavior should be the default as it is much more common in React Native to not use exact versions, it would be awesome if the rnx-align-deps script would provide a flag which if specified, it woud still be checked that the version declared in package.json is within the bound but if it is, won't throw an error.

I've seen #2130 which is somewhat similar (though the reporter didn't really ellaborate on their use-case) and the suggestion there to use a custom preset/config, but in my (our) case we would very much love to use the community config which is a tremendous help in e.g. when updating to a newer React Native version. If align-deps would throw an error if the declared version is outside of the bounds would be very useful in these cases, while going forward, if the dependency is kept within it, won't error out.

Thank you very much for considering my request!
Cheers!

@kelset kelset added feature: align-deps This is related to align-deps needs author feedback labels Feb 21, 2024
@kelset
Copy link
Member

kelset commented Feb 21, 2024

Hey @balazsgerlei - thanks for asking; I'm a bit confused, though. Isn't extending the basic preset enough for you? https://microsoft.github.io/rnx-kit/docs/tools/align-deps#extending-built-in-presets

If you made your own preset and added it in the configuration:

 {
   "name": "my-package",
   ...
   "rnx-kit": {
     "alignDeps": {
       "presets": [
         "microsoft/react-native",
+        "my-preset"
       ],
       "requirements": ["[email protected]"],
       "capabilities": [
         ...
       ]
     }
   }
 }

that should be enough for you to - in your preset - to define the specific deps?

If not, if you could give us more details on your configuration that might help understand your specific need.

re:

it would be great to add it to CI as well, potentially even blocking PR merge if it throws an error.

yes that's how we use it in some internal codebases - it should be fairly straightforward for you to add that -> https://microsoft.github.io/rnx-kit/docs/guides/dependency-management#pull-requests

@balazsgerlei
Copy link
Contributor Author

balazsgerlei commented Feb 21, 2024

Sure thing, I'll try to explain. If I would made my own preset I need to maintain the configuration for dependencies and cannot get "suggestions" from the community ones unless I misunderstood something.

So what I would love if that let's say currently if I use react-native-safe-area-context with React Native 0.72, when I run rnx-align-deps it throws an error unless it is declared as ">=4.5.3 <4.8", even if I declare it with an exact version e.g. "4.7.4" that's within the suggested range. What I would really like is that with specifying a flag (as I understand this should not be the default behavior) to not get an error, but still get one if I e.g. declare it as "4.8.0".

If I would create a custom preset I could say that this dependency should be "4.7.4" but than I would not have any help from the community when upgrading to React Native 0.73. If on the other hand there would be such mode (flag) that I'm suggesting, I wouldn't get an error now as my "4.7.4" declaration satisfies ">=4.5.3 <4.8", but I would get an error if I start the React Native upgrade and forgot to change the version of react-native-safe-area-context.

This would really go a long way I think for anyone who uses exact versions, though I do understand that this is very much the minority in React Native.

Or am I misunderstanding how extending presets work?

BTW for context I started using exact versions as I got fed up with libraries breaking even between patch versions sometimes, on the other hand your tool saved me headaches exactly with react-native-safe-area-context that dropped support for the New Architecture with 4.8.0 but your tool clearly shows that you shouldn't update to it. Similarly that's why I made a contribution to this repo so the same happens with react-native-screens that had a similar breaking change.

I hope I could explain myself, I really appreciate you looking into this and ask me if you need anything to make this happen. Thank you!

@microsoft-github-policy-service microsoft-github-policy-service bot added needs attention Extra attention is needed and removed needs author feedback labels Feb 21, 2024
@tido64 tido64 added the enhancement New feature or request label Feb 21, 2024
@tido64
Copy link
Member

tido64 commented Feb 21, 2024

I think it should be possible to add an option to enable a mode to allow exact versions. Maybe something like this is sufficient:

diff --git a/packages/align-deps/src/diff.ts b/packages/align-deps/src/diff.ts
index 9775f7a8..2f43d5a0 100644
--- a/packages/align-deps/src/diff.ts
+++ b/packages/align-deps/src/diff.ts
@@ -1,10 +1,12 @@
 import { keysOf } from "@rnx-kit/tools-language";
 import type { PackageManifest } from "@rnx-kit/tools-node/package";
+import semverSatisfies from "semver/functions/satisfies";
 import type { Changes } from "./types";

 export function diff(
   manifest: PackageManifest,
-  updatedManifest: PackageManifest
+  updatedManifest: PackageManifest,
+  mode: "exact" | "allow-exact-version"
 ): Changes | undefined {
   const allChanges: Changes = {
     dependencies: [],
@@ -12,6 +14,7 @@ export function diff(
     devDependencies: [],
   };

+  const allowExactVersion = mode === "allow-exact-version";
   const numChanges = keysOf(allChanges).reduce((count, section) => {
     const changes = allChanges[section];
     const currentDeps = manifest[section] ?? {};
@@ -22,7 +25,9 @@ export function diff(
       if (!current) {
         changes.push({ type: "added", dependency, target });
       } else if (current !== target) {
-        changes.push({ type: "changed", dependency, target, current });
+        if (!allowExactVersion || !semverSatisfies(current, target)) {
+          changes.push({ type: "changed", dependency, target, current });
+        }
       }
     }

This will still require some plumbing work e.g., adding an appropriate flag (--allow-exact-version?), plumbing it through, adding tests and documentation.

@balazsgerlei: Is this something you are willing work on? Otherwise, it might take some time before we can have a look at it. It would really help us since you have a repository where you can test this properly as well.

@balazsgerlei
Copy link
Contributor Author

I think I can do it yes, but I would like to ask for a couple of pointers to get started. What you wrote is already very helpful, but can you describe briefly how can I try it out with a React Native project while working on the changes? Thanks!

Please note that I'm more of a native Android & iOS developer, I'm relatively new to React Native.

@tido64
Copy link
Member

tido64 commented Feb 21, 2024

In general, we have a contribution guide: https://github.com/microsoft/rnx-kit/blob/main/CONTRIBUTING.md

If it's missing anything, let us know. For align-deps in particular, you can run yarn bundle inside packages/align-deps and that will bundle the whole thing into lib/index.js. In your project, you can then execute this file directly like so: node /~/rnx-kit/packages/align-deps/lib/index.js

@balazsgerlei
Copy link
Contributor Author

Thank you, the missing link was how to run it, with this I could do the basis of the implementation already. It's not ready yet (e.g. I still need to write unit tests for it) but I tried it out and it works. I created a Draft PR and I would be glad if you could look at it already and tell me if I may went in a wrong direction somewhere: #2984

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature: align-deps This is related to align-deps needs attention Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants