Made pinned dependencies transitive

[LoganGrier]

• Made pinned dependencies transitive resolving this issue
• Moved extract_pinned_dependencies to from bsb_config_parse to bsb_build_util. This enables walk_all_deps to use extract_pinned_dependencies without creating a circular dependency
• Fixed a minor issue in the Troubleshoot a Broken Build section of Contributing.md

[cristianoc]

Left some comments

[cristianoc]

Transitive things are stored in dep_payload, e.g. both package_specs and jsx mode have some transitive behaviour.
Not sure it's necessary to put this information in package context.

[cristianoc]

A perhaps more to the point comment: pinned dependencies inherit package specs from the root project, so a bool is not enough. Assuming that's all that's tracked.

[LoganGrier]

The way that Rescript handles pinned dependencies is pretty weird. In the existing code, pinned-dependencies are just a set of strings. After finding all dependencies using Bsb_build_util.walk_all_deps, the compiler iterates over the queue. All dependencies whose name is in that set is passed as a bsb_package_kind.Pinned_dependency to bsb_ninja_regen.regenerate_ninja. All other dependencies are passed as bsb_package_kind.Dependency. Regardless of whether a dependency is passed as a Pinned_dependency or a Dependency, it is passed with the root project's package_specs and jsx. Any entry in pinned-dependencies which is not the name of a dependency is ignored.

My PR just generalizes the pre-existing behavior. Firstly I've modified Bsb_build_util.walk_all_deps so that it correctly traverses the new dependency graph. Secondly, since bsb_ninja_regen.regenerate_ninja needs to know whether a dependency is pinned, and I already have to calculate that in Bsb_build_util.walk_all_deps, I've modified Bsb_build_util.walk_all_deps to return that information as well.

To address your comments specifically:

• While it's unnecessary to put is_pinned in package_context, I chose to put it there because it makes the code simpler and reduces file reads.
• A bool is enough because it isn't all that's tracked because pinned dependencies are weird.

[cristianoc]

Thanks a lot for doing this and for the explanation.
Would you update CHANGELOG.md as well?

Also, this seems to be safe enough to ship with 10.1 soon, rather than waiting for v11.
If you'd like to have this released sooner rather than later, would you base this on the 10.1 branch rather than master.

Separately, thanks for the comments on how the code for handling dependencies is structured. If you feel like opening a separate PR to clean that up, feel free to do so.

[LoganGrier]

Strictly speaking, this PR is backwards incompatible with 10.0. Any project that assumes non-transitivity would be broken. While I can't think of a situation where someone would make this assumption, since non-transitivity is arguably not a bug, I think it makes sense to wait until the next major version. The workarounds listed in the original issue cover most use cases.

Cleaning Up Pinned Dependencies

I can't spare additional time right now to work on pinned dependencies. That said, I have a few thoughts on how they could be cleaned up.

As I see it, there are two issues with the current system:

1. Entries in pinned-dependencies which are not included in bs-dependencies or bs-dev-dependencies don't raise an exception
2. The name pinned-dependencies suggests similar semantics to bs-dependencies and bs-dev-dependencies, but the semantics are different.

In a way, pinned dependencies are like dev dependencies in that pinned and dev are both traits that may apply to a dependency. Its natural then for both pinned and dev dependencies to be defined with similar syntax. I see two ways of doing this:

1. Eliminate the bs-dev-dependencies and pinned-dependencies config fields and make bs-dependencies a map. Each key in the map is the name of the dependency, and each value defines that dependency's traits. To reduce keystrokes, all dependencies are not dev and not pinned by default. If the same key appears more than once in the map, the compiler returns an error.
2. Eliminate the pinned-dependencies config filed and add bs-pinned-dev-dependencies and bs-pinned-dependencies config fields with the same semantics as bs-dev-dependencies. If any dependency appears more than once in any of the dependency fields, the compiler returns an error.

Here's what that looks like:

Current Syntax

{
  ...
  "bs-dependencies": [
    "a",
    "b"
  ],
  "bs-dev-dependencies": [
    "c",
    "d"
  ],
  "pinned-dependencies": [
    "b",
    "d"
  ]
}

Proposal 1

{
  ...
  "bs-dependencies": {
    "a": {}
    "b": { "pinned": true }
    "c": { "dev": true }
    "d": { "dev": true, "pinned": true }
  }
}

Proposal 2

{
  ...
  "bs-dependencies": [
    "a"
  ],
  "bs-pinned-dependencies": [
    "b"
  ],
  "bs-dev-dependencies": [
    "c"
  ],
  "bs-pinned-dev-dependencies": [
    "d"
  ]
}

Arguments for proposal 1

1. Makes explicit that dev and pinned are traits of dependencies
2. Easier to extend to add other traits down the line. For example whether to report errors, or respect warn-error. If proposal 2 is extended to add other traits, the number of config fields grows exponentially with the number of traits.

Arguments for proposal 2

1. Extends the syntax of package.json. Potentially more intuitive for new users.

[LoganGrier]

Thinking about this some more, the underlying problem that pinned dependencies solve is that compiling a project overwrites the compilation results of its dependencies. I see two ways of addressing this

1. If a dependency has already been built, reuse the build result. It's common enough to want to rebuild dependencies as well so there should be a CLI flag to do this.
2. When building dependencies, the compiler could output the build result inside the root project directory instead of the dependency project directory. This way, there is no need to delete the existing build. As a bonus, this would eliminate race conditions when building projects in parallel.

While option 1 sounds simple, I can see potential complexity stemming from different compiler versions and configurations being used across different projects. Option 2 on the other hand seems pretty straightforward. The main advantage I see of option 1 over 2 is performance.

[cristianoc]

Thanks. Merging now.