Rework publishing to work with all popular package managers #674
Conversation
🦋 Changeset detectedLatest commit: 7608211 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| [ | ||
| ...publishTool.args, | ||
| opts.cwd, | ||
| "--json", |
There was a problem hiding this comment.
Looks like if yarn 2+ does not support --json, the argument should be conditionally added
There was a problem hiding this comment.
yes, thank you for spotting that - this is an obvious oversight on my part, I knew that this ain't supported as it makes me handle yarn berry in a different way just a few lines later
just out of curiosity - is using an unknown flag makes yarn throw an error?
There was a problem hiding this comment.
Unfortunately yes, just tested with your current P/R to be sure (yarn 3.2.0-rc.5):
soluble-io/cache-interop#222 (comment)
BTW I love the butterfly in the yarn output, made me smile :)
🦋 error Unknown Syntax Error: Extraneous positional argument ("/home/sebastien/github/cache-interop/packages/cache-redis").
There was a problem hiding this comment.
I think you have tested the other case 😅 but this one doesn't work either:
Unknown Syntax Error: Unsupported option name ("--json").
There was a problem hiding this comment.
Yep 😅, I've been struggling a bit with how to build your P/R to use it...
But that definitely happened and it was nice to see.
There was a problem hiding this comment.
I will make sure to setup snapshot releases for PRs before we go into re-testing this after I apply the suggested changes
| ["publish", opts.cwd, "--json", ...publishFlags], | ||
| [ | ||
| ...publishTool.args, | ||
| opts.cwd, |
There was a problem hiding this comment.
opts.cwd
I don't think it will work with yarn 2+, not sure about the other package managers (https://yarnpkg.com/cli/npm/publish has no reference to path)
I think this should work
const args = [
...publishTool.args,
// "--json", // <- assuming here we add --json for naything !== yarn 2+
...publishFlags,
...publishTool.flags
];
let { code, stdout, stderr } = await spawn(
publishTool.name,
args,
{
// Here we can send the current working directory
cwd: opts.cwd,
env: Object.assign({}, process.env, envOverride)
}
);Just tested it and it works on yarn 3. But I'm wondering if it would be cleaner to extract the command and add tests on it. I'll have a look tomorrow to see if I can help
There was a problem hiding this comment.
Ah, yes - good catch. I got Unknown Syntax Error: Extraneous positional argument ("packages/test"). with this.
I think it's safe to just use { cwd: opts.cwd } for all package managers.
There was a problem hiding this comment.
Just tested it and it works on yarn 3. But I'm wondering if it would be cleaner to extract the command and add tests on it. I'll have a look tomorrow to see if I can help
Tests for this whole thing would be great. I'm just unsure how to test this best - we really should have some e2e tests here because there are so many little details hidden within those publish commands that we really should invoke those and verify what can be observed somehow.
There was a problem hiding this comment.
Totally. I would be lazy and just unit test something in the lines of
type Params= {
pm: 'pnpm' | 'yarn' | 'npm',
pmVersion?: string,
tag?: string,
access?: 'public' | 'private',
otp?: string
}
const getPublishSpawnParams = (params: Params): {cmd: string, args: string[]} => {
///....
}
describe('when pnpm', () => {
it('should return a pnpm..', () => {
expect(getPublishSpawnParams({pm: 'pnpm', pmVersion: '6.4' tag: 'main', access: 'public', otp: 'xxx'}).toStrictEqual({
cmd: 'pnpm',
args: ['publish', '--access', 'public', '--tag', 'main', '--json', '--no-git-checks']
})
});
}
describe('when yarn < 2', () => {
it('should return a npm publish..', () => {
expect(getPublishSpawnParams({pm: 'yarn', pmVersion: '8.0', tag: 'main', access: 'public', otp: 'xxx'}).toStrictEqual({
cmd: 'npm',
args: ['publish', '--access', 'public', '--tag', 'main' '--otp', 'xxx']
})
});
}
describe('when yarn 2+', () => {
it('should return a yarn..', () => {
expect(getPublishSpawnParams({pm: 'yarn', pmVersion: '2.4', tag: 'main', access: 'public', otp: 'xxx'}).toStrictEqual({
cmd: 'yarn',
args: ['npm', 'publish', '--access', 'public', '--tag', 'main']
})
});
}
describe('when yarn 3.2', () => {
it('should return a yarn..', () => {
expect(getPublishSpawnParams({pm: 'yarn', pmVersion: "3.2', tag: 'main', access: 'public', otp: 'xxx'}).toStrictEqual({
cmd: 'yarn',
// I saw that you've opened a --otp option in yarn, so why not ?
args: ['npm', 'publish', '--access', 'public', '--tag', 'main', '-otp', 'xxx']
})
});
}
// + some extras Not so sure about how to mock spawn, so that might make sense
I'm bad with the names hey :)
There was a problem hiding this comment.
I'm just checking and you're totally right so much small details, that's insane :)
So definitely I would live well with unit tests cause e2e looks a huge effort, especially with minor versions.
I would start with some specs for the most common cases (npm, yarn 1/2+, pnpm 6) and live with a "dirty" function that can be improved later (with things like https://www.npmjs.com/package/compare-versions to adapt the result)
|
@belgattitude thank you very much for testing this ❤️ I will make necessary changes here soon, I would only want to land this first #676 so I could rebase this one on top of it. |
Andarist
force-pushed
the
publish-refactor
branch
from
89a70f9
to
5cae4f5
Compare
…e path as argument to the command. This makes sure that it will work with Yarn Berry Co-authored-by: Sébastien Vanvelthem <belgattitude@users.noreply.github.com>
Co-authored-by: Sébastien Vanvelthem <belgattitude@users.noreply.github.com>
|
@belgattitude I've rebased this and applied your suggestions. I've also created a PR to add snapshot releases (here). This can only be merged in when we confirm that it has been done in a secure way, so I don't want to rush it. If you could retest this PR "manually" (like you have done before) - I would highly appreciate it. |
|
Hey @Andarist thanks a lot. Here it is yarn changeset publishThe command is not complaining anymore with yarn 3.2.0-rc.6.
PS: I've added the log here: https://github.com/atlassian/changesets/pull/682/files just to be sure. That said it's not a full publish, I'll need --otp for that and can only publish my repo from a gh action for now. So all look fine. My concerns are related to
Of course all is up to you, I come from different ecosystem so my reflexes often lack perspective and love to learn and share in general. Let me know I still have few free days, and I'll gladly help. |
That's probably a good call - anything that you would like to see logged beside the information added in your PR here?
I agree that we need some tests here - I'm just on the fence when it comes to unit tests here. The logic itself is pretty straightforward - the only problem is that I'm not always sure what exact combination of options has to be passed in a specific situation. And when it comes to that I think that comments can be more valuable than unit tests. I just kinda feel that I would be that more confident about this logic if I would have those unit tests. I'm not a testing expert though and probably many people would say that this would be worthwhile - I'm just not feeling that. Maybe @mitchellhamilton would have some additional opinion about this.
Just to make sure that we are on the same page - everything looks OK now from your end, it's just that publish fails because OTP is required and you are not using the automation token because you are running the publish script locally and not on the CI. Right? |
tbh, what I'd really like is tests that run with all the different package managers against a local registry (probably https://verdaccio.org) with a way to run the tests against actual registries. |
|
Let's move the tests-related discussion to the dedicated thread. I don't think this is sort of out of the scope of this PR and should be addressed separately. |
New to the project so I don't know but I would follow the concerns from #674 (review) @mitchellhamilton
Yep. Don't want to mess too much my repo 😄. A canary of changeset can help for me to test it out fully, but as the "yarn command" is okay, I feel I can be confident enough the tool will do its job.
Just want to express something about unit. I'm certainly not trying to create a debate, just sharing and if it's not useful, please forget.
If you like I could add an example test file in a separate P/R... The only thing is that I don't really know node, so a reviewer/guide will be needed 😄 Hey thanks again |
|
@Andarist a little addition. I'm not sure you're aware of it. When I faced the issue with yarn/pnpm workspace: protocol, I just did a quick hack for my own use case. Rather than change npm for publication, I intercepted the pack step (yarn pack, pnpm pack) to generate the archive. see here #585 (comment). I'm thinking of it now, I guess it's a bit late to tell and I don't want to add confusion here. So feel free to ignore, but it might help to harmonize and have one tool to test for publishing. |
|
@mitchellhamilton any updates? |
|
@ragingwind I still have some minor things to adjust here and to figure out the authentication story in Also, note that you can try out some prerelease versions that include this PR and get back to me with the feedback. This would actually be very helpful. If you want to do that - let me know and I will prepare some installation instructions for you (please just mention which package manager are you currently using). |
|
@Andarist Thank you. It would be great I could use it with prerelease. We use yarn@3.1.0 now |
|
@Andarist definitely looking forward to this PR, we are attempting to upgrade our monorepo from yarn v1 to yarn v3 and would like to also retain our conventional-commits/semantic releases that lerna used to give but is no longer compatible with yarn berry |
|
@ragingwind I would say that at the moment using this packed version from CodeSandbox CI is the easiest: Since you are using Yarn 3 you'd have to use this Yarn plugin to resolve requested dependencies correctly. You could also try installing this version from npm: but it's a little behind the one available on CodeSandbox CI - and any future prereleases like that will only be available through CodeSandbox CI so that's what I recommend now. Note that if you are also using
@ilyaGurevich note that how Changesets work is slightly different from that - the generated changelog updates etc are not based on commit messages but on the committed changeset |
|
Also notice that |
|
@Andarist do you need some additional help to finish it? |
|
hw is this pr going and when will i be able to use this? atm moment i really need this because my publishing automation for my project is broken due to yarn not being supported. |
|
Just checking on this PR, keen to see it finished. @Andarist if you need any help (or sponsorship) to see it through, I'd be happy to help, it's an awesome tool otherwise. 🦋 |
|
are there any updates on this, we really miss the great capabilities of |
|
how about this issue progress? really need this feature without any hack. |
➤ YN0000: Done in 7s 668ms |
|
@Andarist could you please rebase with the main branch? I need the latest main branch changes in the csb/snapshot releases. |
|
I've been trying out this PR with @qnighy's patch and with yarn I get the following error message when running I traced this to the fact that If I simply short circuit this function and |
|
Just bumped into the fact that for A similar logic than changesets/packages/cli/src/commands/publish/npm-utils.ts Lines 189 to 197 in 7608211 Here's an updated version of the above patch which works in yarn {
"name": "package",
"version": "0.0.0",
"devDependencies": {
"@changesets/cli": "patch:@changesets/cli@npm%3A2.26.0#~/.yarn/patches/@changesets-cli-npm-2.26.0-49d5c5f72d.patch"
},
"packageManager": "yarn@4.0.0-rc.45",
"resolutions": {
"@changesets/cli@^2.26.0": "patch:@changesets/cli@npm%3A2.26.0#./.yarn/patches/@changesets-cli-npm-2.26.0-49d5c5f72d.patch"
},
}.yarn/patches/@changesets-cli-npm-2.26.0-49d5c5f72d.patch |
fixes #598
fixes #432
at the moment this is being opened as a draft, to gather initial feedback
The goal of this PR is to delegate as much as possible to yarn berry / pnpm so they can do their magic with rewriting package manifests and stuff
TODO:
.npmrccc @schickling @zkochan (might be interesting to you)