[rush] publishing prereleases breaks inter-repo dependencies

raygesualdo commented 5 years ago

Hey all, thanks for your work on rush! We just changed over from Lerna recently and it's been a great productivity booster. When we switched over, our prerelease publishing process broke and it took me a while to figure out why. I've documented what's going on below.

Expected Behavior

When I publish packages using the --prerelease CLI flag, a package (e.g. packageA) with a changelog entry should be version bumped, given the --prerelease suffix specified, and published to the registry. Any of its dependencies (e.g. packageB) in the repo that do not have changelog entries should not be version bumped and published, and the references to those dependencies should not be changed in packageA.

Observed Behavior

packageA gets incremented and published correctly. But its dependency on packageB has been incremented to a "prerelease" version that rush didn't publish to the registry. This can be seen by viewing the output from npm info for the packages.

packageA after a normal release

Note how it's at v2.0.0 and its dependency packageB is at v2.0.0 as well.

packageA after a subsequent prerelease

Note how it's at v2.0.1-test and its dependency packageB is listed at v2.0.0-test. So the -test suffix was applied to the packageB dependency entry in packageA's package.json file even though it wasn't involved in the publish. We can see that packageB was not updated at all by viewing it's npm info output:

Steps to Reproduce

Go to the repro repo I set up, clone it, and follow the instructions. Let me know if something doesn't work.

Proposed Solution

Either refactor the prerelease code to not add the prerelease suffix to packages that aren't being published, or publish all a changed package's dependencies alongside it with the prerelease suffix applied. I'm sure there are other ways to accomplish this. These were the two that seemed the most straightforward.

Environment

rush: 5.5.4
pnpm: 2.15.1
node: 10.13

raygesualdo commented 5 years ago

For posterity, here's @octogonz response from gitter:

I'm not an expert on Rush's publishing feature (the person who designed it is no longer active), but what you described sounds like it may be the expected behavior.

As I recall the --prerelease option is mainly used with "version policies" (i.e. groups of packages that publish together as a set, typically all with the same version number), and so --prerelease is used to publish a release candidate of the set of packages.

However, your use case and expectations also seem pretty reasonable. Longer term, this year we've been planning to do a sprint where we revamp the entire design for publishing and make it simpler and easier to understand, based on everything we learned during the evolution of the current implementation. But for the short term, the quickest fix might be to add another option that makes --prerelease behave how you described.

For example --partial-prerelease or somesuch. Maybe you could suggest something?

qm3ster commented 5 years ago

I understand how the --partial-prerelease behavior could be not implemented, but how did it the current behavior end up the way it is? Why aren't the unchanged-but-bumped packages published?

octogonz commented 5 years ago

I understand how the --partial-prerelease behavior could be not implemented, but how did it the current behavior end up the way it is? Why aren't the unchanged-but-bumped packages published?

My guess would be that the --prerelease-name flag hasn't been tested in a long time. The rush publish command-line confusingly mixes together two different publishing designs:

rush pubish does everything: (deprecated) In the original design, rush publish bumped versions, committed the result to Git, AND published the result to the NPM registry. This design caused a lot of problems and was eventually deprecated. But we couldn't remove it because some repos still try to use it.
rush version then rush publish: In the revised design, there are two CI jobs: The first job (1) bumps versions, (2) commits the result to Git, (3) builds everything to produce a tarball artifact. The second job (an Azure DevOps "release") publishes the result to NPM. This approach has a couple major advantages: First, if publishing fails halfway through due to a network error, it can be retried without having to redo the build. Second, when testing a release (e.g. using Verdaccio or a private registry), we can test the exact same bits that will eventually be published to the real NPM registry.

Some other options were introduced while evolving from the old approach to the new one. Also, we started using version-policies.json instead of the command-line to supply information and drive the set of packages to be published. Lastly, the people who wrote the publishing code have moved on to other things, so the current Rush owners basically just rely on configuration recipes without a deep understanding of the details. Speaking very frankly here. :-P

As a result, although it does get the job done, publishing is currently the most confusing aspect of Rush. We really want to make it better. Last fall we opened issue https://github.com/Microsoft/web-build-tools/issues/904 and then went off and had a couple meetings where we came up with a plan to simplify the design, while also making it more flexible, and incorporating various community requests. I've been planning to drive the implementation myself this summer. First I need to wrap up the API Extractor 7 release and write up the TSDoc spec, but otherwise this would be my next priority as far as OSS projects. It affects me personally because I get questions about it all the time, and I wish I could give better answers heheh.

Anyway, in the meantime, if people have reasonable-looking PRs that make the current rush publish design work better, we'll take 'em.

raygesualdo commented 5 years ago

Thanks for the background @octogonz. I should have a PR with the --partial-prerelease changes up today.

qm3ster commented 5 years ago

Great to know that this project is heading towards the latter design. It makes perfect sense, and is similar to what people do semi-manually with semantic release and such. I ended up setting up the current publishing, but the trial and error process was rather frustrating since either half could fail. Another advantage of the split approach is that now only the bumping stage needs push access to the code, and only the publish stage needs npm token.

raygesualdo commented 5 years ago

With the v3 release, Lerna split the versioning and publishing processes too, presumably for the same reasons. Having those split up would be 💯.

octogonz commented 5 years ago

Another advantage of the split approach is that now only the bumping stage needs push access to the code, and only the publish stage needs npm token.

I forgot to mention that! From a security standpoint, it's very important to isolate CI jobs that have access to sensitive service credentials. Also, jobs that commit/publish/deploy may need to run on a completely different VM pool.

raygesualdo commented 5 years ago

Update: doing some final testing to make sure the full publish workflow runs as expected, but code changes are made and working.

microsoft / rushstack