-
Notifications
You must be signed in to change notification settings - Fork 91
Description
--no-bump/bump: false was added in #376, but some nuances of the implementation don't appear to have been fully thought through (partly due to some misleading function names/docs and lack of tests) and still have inconsistent/buggy behavior today...
It appears that even in the initial implementation, although the package versions on disk are correctly not updated, other parts of the process use the in-memory bumped versions: for example, in publishToRegistry's validatePackageVersions call, it validates whether the bumped versions exist in the registry (not the on-disk versions that will actually be published).
It's also a bit odd that the list of packages to be published (getPackagesToPublish) is determined by the list of in-memory bumped packages based on current change files. Presumably the intent was for --no-bump to allow doing the npm publish and git update in separate steps (so the change files are relevant) but it wasn't documented.
Overall, there needs to be a detailed look at the behavior of this option to ensure consistency/coherence throughout the publishing process, and ensure the right objects/values are being used at the right times. (One possible/partial approach is to update bumpInMemory to calculate changes and dependents but skip modifying package infos.) Also, the intent/behavior of the option needs to be documented and tested in more detail.
(History: gatherBumpInfo originally didn't do the in-memory bump, but this prior PR moved the bumpInPlace call from performBump into gatherBumpInfo while keeping the old function names, and the doc comments weren't updated. This seems to have led to later confusion about which function did what, including code which unnecessarily clones the bumpInfo during performBump even though that function doesn't mutate it.)