Made pinned dependencies transitive (#5722)

* Made pinned dependencies transitive. Moved extract_pinned_dependencies to bsb_build_util. Minor change to clean build script. Signed-Off-By: Logan Grier <[email protected]>

* Eliminated 'npm install' in transitive_pinned_dependency tests. Signed-Off-By: Logan Grier <[email protected]>

* Deleted transitive pinned dependency test package-lock.json files. Signed-Off-By: Logan Grier <[email protected]>

* Added entry in changelog. Signed-Off-By: Logan Grier <[email protected]>

Author: LoganGrier

CHANGELOG.md

@@ -19,6 +19,11 @@
   - `rescript convert <reason files>`
 - Remove obsolete built-in project templates and the "rescript init" functionality. This will be replaced by the create-rescript-app project that is maintained separately.
 
+#### :boom: Breaking Change
+
+- Made pinned dependencies transitive: if *a* is a pinned dependency of *b* and *b* is a pinned dependency of *c*, then *a* is implicitly a pinned dependency of *c*.
+- This change is only breaking if your build process assumes non-transitivity. Few if any builds do. In the typical case where you build your monorepo by running `rescript build` on each package in your repo, you don't need to make any changes. There is no way of building with the old, non-transitive behavior.
+
 # 10.1.0-rc.1
 
 ## :rocket: New Feature

CONTRIBUTING.md

@@ -76,6 +76,7 @@ Usually whenever there's some issues with missing files, incompatible interfaces
 
 ```sh
 ./scripts/ninja.js clean # remove files not in version control
+npm install
 ./scripts/ninja.js config
 ./scripts/ninja.js build
 ```

jscomp/bsb/bsb_build_util.ml

@@ -130,7 +130,7 @@ let ( |? ) m (key, cb) = m |> Ext_json.test key cb
 
 type top = Expect_none | Expect_name of string
 
-type package_context = { proj_dir : string; top : top }
+type package_context = { proj_dir : string; top : top; is_pinned: bool }
 
 (**
    TODO: check duplicate package name
@@ -146,6 +146,13 @@ type package_context = { proj_dir : string; top : top }
 let pp_packages_rev ppf lst =
   Ext_list.rev_iter lst (fun s -> Format.fprintf ppf "%s " s)
 
+let extract_pinned_dependencies (map : Ext_json_types.t Map_string.t) : Set_string.t =
+  match Map_string.find_opt map Bsb_build_schemas.pinned_dependencies with
+  | None -> Set_string.empty
+  | Some (Arr { content }) ->
+      Set_string.of_list (get_list_string content)
+  | Some config -> Bsb_exception.config_error config "expect an array of string"
+
 let rec walk_all_deps_aux (visited : string Hash_string.t) (paths : string list)
     ~(top : top) (dir : string) (queue : _ Queue.t) ~pinned_dependencies =
   let bsconfig_json = dir // Literals.bsconfig_json in
@@ -174,9 +181,9 @@ let rec walk_all_deps_aux (visited : string Hash_string.t) (paths : string list)
       if Hash_string.mem visited cur_package_name then
         Bsb_log.info "@{<info>Visited before@} %s@." cur_package_name
       else
-        let explore_deps (deps : string) =
+        let explore_deps (deps : string) pinned_dependencies =
           map
-          |? ( deps,
+          |? ( deps,                
                `Arr
                  (fun (new_packages : Ext_json_types.t array) ->
                    Ext_array.iter new_packages (fun js ->
@@ -194,13 +201,24 @@ let rec walk_all_deps_aux (visited : string Hash_string.t) (paths : string list)
              )
           |> ignore
         in
-        explore_deps Bsb_build_schemas.bs_dependencies;
+        let is_pinned = match top with
+        | Expect_name n when Set_string.mem pinned_dependencies n -> true
+        | _ -> false
+        in
+        let pinned_dependencies = match is_pinned with 
+        | true -> 
+          let transitive_pinned_dependencies = extract_pinned_dependencies map
+          in
+          Set_string.union transitive_pinned_dependencies pinned_dependencies
+        | false -> pinned_dependencies
+        in
+        explore_deps Bsb_build_schemas.bs_dependencies pinned_dependencies;
         (match top with
-        | Expect_none -> explore_deps Bsb_build_schemas.bs_dev_dependencies
-        | Expect_name n when Set_string.mem pinned_dependencies n ->
-            explore_deps Bsb_build_schemas.bs_dev_dependencies
+        | Expect_none -> explore_deps Bsb_build_schemas.bs_dev_dependencies pinned_dependencies
+        | Expect_name _ when is_pinned ->
+            explore_deps Bsb_build_schemas.bs_dev_dependencies pinned_dependencies
         | Expect_name _ -> ());
-        Queue.add { top; proj_dir = dir } queue;
+        Queue.add { top; proj_dir = dir; is_pinned } queue;
         Hash_string.add visited cur_package_name dir
   | _ -> ()
 

jscomp/bsb/bsb_build_util.mli

@@ -82,7 +82,9 @@ type result = { path : string; checked : bool }
 *)
 val resolve_bsb_magic_file : cwd:string -> desc:string -> string -> result
 
-type package_context = { proj_dir : string; top : top }
+type package_context = { proj_dir : string; top : top; is_pinned: bool }
+
+val extract_pinned_dependencies: Ext_json_types.t Map_string.t -> Set_string.t
 
 val walk_all_deps :
   string -> pinned_dependencies:Set_string.t -> package_context Queue.t

jscomp/bsb/bsb_config_parse.ml

@@ -142,13 +142,6 @@ let extract_ignored_dirs (map : json_map) : Set_string.t =
       Set_string.of_list (Bsb_build_util.get_list_string content)
   | Some config -> Bsb_exception.config_error config "expect an array of string"
 
-let extract_pinned_dependencies (map : json_map) : Set_string.t =
-  match map.?(Bsb_build_schemas.pinned_dependencies) with
-  | None -> Set_string.empty
-  | Some (Arr { content }) ->
-      Set_string.of_list (Bsb_build_util.get_list_string content)
-  | Some config -> Bsb_exception.config_error config "expect an array of string"
-
 let extract_generators (map : json_map) =
   let generators = ref Map_string.empty in
   (match map.?(Bsb_build_schemas.generators) with
@@ -286,7 +279,7 @@ let interpret_json ~(package_kind : Bsb_package_kind.t) ~(per_proj_dir : string)
               Bsb_build_schemas.bs_dev_dependencies
         | Dependency _ -> []
       in
-      let pinned_dependencies = extract_pinned_dependencies map in
+      let pinned_dependencies = Bsb_build_util.extract_pinned_dependencies map in
       match map.?(Bsb_build_schemas.sources) with
       | Some sources ->
           let cut_generators =
@@ -357,5 +350,5 @@ let deps_from_bsconfig () =
   | Obj { map } ->
       ( Bsb_package_specs.from_map ~cwd:Bsb_global_paths.cwd map,
         Bsb_jsx.from_map map,
-        extract_pinned_dependencies map )
+        Bsb_build_util.extract_pinned_dependencies map )
   | _ -> assert false

jscomp/bsb/bsb_world.ml

@@ -56,11 +56,10 @@ let make_world_deps cwd (config : Bsb_config_types.t option)
      );
      close_out oc ; *)
   queue
-  |> Queue.iter (fun ({ top; proj_dir } : Bsb_build_util.package_context) ->
+  |> Queue.iter (fun ({ top; proj_dir; is_pinned } : Bsb_build_util.package_context) ->
          match top with
          | Expect_none -> ()
          | Expect_name s ->
-             let is_pinned = Set_string.mem pinned_dependencies s in
              if is_pinned then print_endline ("Dependency pinned on " ^ s)
              else print_endline ("Dependency on " ^ s);
              let lib_bs_dir = proj_dir // lib_artifacts_dir in

jscomp/build_tests/transitive_pinned_dependency1/.gitignore

@@ -0,0 +1 @@
+lib

jscomp/build_tests/transitive_pinned_dependency1/a/bsconfig.json

@@ -0,0 +1,27 @@
+{
+    "name": "a",
+    "namespace": true,
+    "sources": [
+        {
+            "dir": "src"
+        },
+        {
+            "dir": "tests",
+            "type": "dev"
+        }
+    ],
+    "package-specs": {
+        "module": "commonjs",
+        "in-source": false
+    },
+    "warnings": {
+        "error": true
+    },
+    "suffix": ".mjs",
+    "bs-dependencies": [
+        "b"
+    ],
+    "pinned-dependencies": [
+        "b"
+    ]
+}

jscomp/build_tests/transitive_pinned_dependency1/a/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "a",
+  "version": "1.0.0"
+}

jscomp/build_tests/transitive_pinned_dependency1/a/src/src.res

@@ -0,0 +1 @@
+Js.Console.log("src")

jscomp/build_tests/transitive_pinned_dependency1/a/tests/test.res

@@ -0,0 +1 @@
+Js.Console.log("test")

jscomp/build_tests/transitive_pinned_dependency1/input.js

@@ -0,0 +1,9 @@
+//@ts-check
+var child_process = require("child_process");
+var assert = require("assert");
+var fs = require("fs")
+var rescript_exe = require("../../../scripts/bin_path").rescript_exe;
+
+console.log(child_process.execSync(rescript_exe, { encoding: "utf8", cwd: "./a" }));
+
+assert(fs.existsSync("./node_modules/c/lib/js/tests/test.mjs"), "dev files of module 'c' were not built by 'a' even though 'c' is a transitive pinned dependency of 'a' through 'b'")

jscomp/build_tests/transitive_pinned_dependency1/node_modules/b/bsconfig.json

@@ -0,0 +1,8 @@
+{
+  "name": "top",
+  "workspaces": [
+    "a",
+    "b",
+    "c"
+  ]
+}

jscomp/build_tests/transitive_pinned_dependency1/node_modules/b/package.json

@@ -0,0 +1 @@
+lib

jscomp/build_tests/transitive_pinned_dependency1/node_modules/b/src/src.res

@@ -0,0 +1,24 @@
+{
+    "name": "a",
+    "namespace": true,
+    "sources": [
+        {
+            "dir": "src"
+        },
+        {
+            "dir": "tests",
+            "type": "dev"
+        }
+    ],
+    "package-specs": {
+        "module": "commonjs",
+        "in-source": false
+    },
+    "warnings": {
+        "error": true
+    },
+    "suffix": ".mjs",
+    "bs-dependencies": [
+        "b"
+    ]
+}

jscomp/build_tests/transitive_pinned_dependency1/node_modules/b/tests/test.res

@@ -0,0 +1,4 @@
+{
+  "name": "a",
+  "version": "1.0.0"
+}

jscomp/build_tests/transitive_pinned_dependency1/node_modules/c/bsconfig.json

@@ -0,0 +1 @@
+Js.Console.log("src")

jscomp/build_tests/transitive_pinned_dependency1/node_modules/c/package.json

@@ -0,0 +1 @@
+Js.Console.log("test")

jscomp/build_tests/transitive_pinned_dependency1/node_modules/c/src/src.res

@@ -0,0 +1,9 @@
+//@ts-check
+var child_process = require("child_process");
+var assert = require("assert");
+var fs = require("fs")
+var rescript_exe = require("../../../scripts/bin_path").rescript_exe;
+
+console.log(child_process.execSync(rescript_exe, { encoding: "utf8", cwd: "./a" }));
+
+assert(!fs.existsSync("./node_modules/c/lib/js/tests/test.mjs"), "dev files of module 'c' were built by 'a' even though 'c' is not a pinned dependency of 'a'")