fix(node-workspace): Add update-peer-dependencies option that also updates peer dependencies. Fixes #1943

[TimothyJones]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #1943 🦕

Note: There was already a test to make sure that peerDependencies were NOT updated. I can't think of a reason that this behaviour would be desirable, so I just updated the test to expect the peerDependencies to be updated.

Also, the tests don't full pass locally, because this function doesn't alter dates from my locale (eg 10/11/2023). I didn't fix this, but I believe the tests will totally pass in CI.

[chingor13]

Thank you for this. This was intentionally put in, I'm not entirely sure why, but it is tested behavior. We don't want to break any existing users who are using this without taking a breaking change.

Instead, we could add a configuration to the node-workspace plugin, to includePeerDependencies and keep the default to false. This way we don't break users and you can get your peer dependencies updated.

In the next major version bump, we can consider switching the default to true because we can make those kinds of breaking changes in a major version bump.

[TimothyJones]

Thanks for the review! I've been out sick, but I'll make the change this week and tag you when I've done so.

[TimothyJones]

@chingor13 I've added an option update-peer-dependencies, following the pattern that the other node workspace option (always-link-local) used. There's documentation, and new tests (copies of existing tests with different expectations).

It feels like it would be better to make options for plugins a first-class concept rather than handling them in the same place as global options, as then it would be easier to define plugins outside the main release-please source. I think that kind of change is out of scope for this PR, though.

Thanks again for all your work on this tool!

[chingor13]

It feels like it would be better to make options for plugins a first-class concept rather than handling them in the same place as global options, as then it would be easier to define plugins outside the main release-please source. I think that kind of change is out of scope for this PR, though.

The schema allows for customizing plugins with an object in the config, for example:

{
  "plugins": [
    {
      "type": "node-workspace",
      "merge": false
    }
  ]
}

This could be a place to put the alwaysLinkLocal and includePeerDependencies options rather than using the top-level manifest configs. These options are loaded in the plugin factory implementation

Also note that I am currently refactoring the NodeWorkspace implementation in #2117 to drop @lerna-lite/core because it breaks our action implementation somehow.

[TimothyJones]

Thanks. I'd be happy to do that refactor in a followup PR, unless you think it should be part of this change?

[TimothyJones]

Also note that I am currently refactoring the NodeWorkspace implementation in #2117 to drop @lerna-lite/core because it breaks our action implementation somehow.

I'm not sure if you're asking me to do something here - if you want to merge this one (and potentially release it) before your refactor lands, then you'd get tests for the peerDependencies case (although the implementation that lands in this PR would obviously change during the refactor).

Could you clarify what the status is?

[TimothyJones]

Build failure seems unrelated to this PR

[chingor13]

Thanks. I'd be happy to do that refactor in a followup PR, unless you think it should be part of this change?

Please do it as part of this PR. We always want main to be in a releasable state and we don't want to introduce the top level config option and have to remove it (breaking change) or support it until the next major version.

If #2117 lands first, I can deal with the merge conflicts in this PR to make the new tests pass. If this lands first, then I'll update my PR with the new feature.

[TimothyJones]

We always want main to be in a releasable state

Respectfully, I don't think this PR is unreleasable. I followed the existing pattern for existing node-workspace options, so it seems a bit unfair to describe it as unreleasable.

I think the possibilities are:

1. Refactor the options here, moving both alwaysLinkLocal and includePeerDependencies to the plugin factory. As you point out, that would be a breaking change, since alwaysLinkLocal would no longer be a top level option.

2. Put only includePeerDependencies in the plugin factory. I feel this option would be worse - it would introduce inconsistent patterns in the code, and inconsistent behaviour in the way users specify options.

3. Do the change later, meaning that the pattern doesn't get worse. As you point out, that would imply a future breaking change when the options are moved out - but this is the state your code is in already, even if this PR wasn't necessary or never lands.

I think refactoring the node-workspace options is more appropriate task for a maintainer (which I am not), and I also think it's unrelated to the work in this PR, which ideally shouldn't introduce breaking changes. This is why I went for option 3 when I added includePeerDependencies.

This PR fixes a bug. By request I put the bug fix behind a config flag, so as not to break anyone relying on the old behaviour.

I was careful to only raise a mergeable PR, and careful to follow the existing patterns in this code. If you feel that it's still not mergeable in this state, please feel free to push commits before merging.

[chingor13]

So it turns out that the root always-link-local option is not even reaching the node-workspace plugin as currently written. It looks like this was broken since v13.0.0 when we did the initial factory and strategy refactor. So, as written, this PR also won't propagate the include-peer-dependencies root option.

I need to rethink about the order of how to merge all these changes/fixes together.

[TimothyJones]

If #2117 lands first, I can deal with the merge conflicts in this PR to make the new tests pass

I see that it has landed - yes please, that would be great!

[chingor13]

Will refactor the config option to put into node-workspace options rather than at the top level in a separate PR.