Repository navigation
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 |
emmatown
left a comment
emmatown
left a comment
There was a problem hiding this comment.
Maybe we should log stdout and stderr when publish fails so people can debug when something goes wrong like we fail to parse the json? (and maybe even land that and the getLastJsonObjectFromString stuff separately to package manager changes?)
| [ | ||
| ...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. |
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 <[email protected]>
Co-authored-by: Sébastien Vanvelthem <[email protected]>
|
@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. |
|
Any interest in picking this work back up? We love changesets at the Adobe Spectrum CSS project but we're leveraging yarn 4.1.1 and the workspaces:^ syntax and would love to see even a small update to using |
|
any plan of this? |
|
I would highly recommend switching to https://github.com/monoweave/monoweave for Yarn-based projects. I had a great experience with this tool, fills all my versioning/publishing needs in a monorepo. |
|
waiting changesets support |
|
Hey @Andarist sorry for the ping, I got surprised by this pretty bad today 😅 Are you planning on picking this PR up again? Else, one option I can think of is just to remove |
|
+1 |
|
Hey @Andarist, at my company we stumbled upon this issue a few weeks ago and it cost us quite some outages and bugs due to duplicate versions or incorrect resolutions in yarn.lock. I'm surprised to find out that this issue has been there since 2021. It hasn't been fixed and it is also not in the docs anywhere. It could have saved us a lot of headache to know that yarn publish isn't supported. In my opinion, this deserves a warning on your main README file. Fortunately, we managed to fix this issue with the changes in this PR. So far I didn't get your feedback there. Could you let me know why isn't it an option to merge it? Most importantly, if you are not fixing this issue, do you have at least a recommendation for people stuck in this problem? At my company, we chose changesets and are willing to help if the effort is manageable. Please let us know if there's anything we can do. And please document this because others should know before they choose this tool. |
|
Would love to see this get landed. Could @transloadit sponsor this effort perhaps? |
|
I'm all for getting this fixed as it is causing us a lot of headaches as well, but this is a good time to reflect on the current state of open source. Developers of open source libraries are getting paid close to nothing and large corporations are profiting from their work. While we wait for this PR to hopefully resolve, please have a look at Dylan Beattie's excellent presentation at NDC and go tell your bosses to help out developers like these. https://github.com/sponsors/explore |
|
Is this an ai response? I actually offered to sponsor this. And we allocate 80% of revenue to open source at Transloadit |
There are obviously more people visiting this PR than just yourself. While it is great to hear about how you help the open source community, this is not very common for companies, especially those where IT is not considered part of their core business, to do. I posted my message in the hopes that other frustrated developers that might come across this PR have a look at the presentation and reflect while we wait |
|
+1 |
|
Having this exact same bug when using workspaces in bun. How can we help to see this fixed? |
|
+1 |
|
+1 |
|
Hi all, I was able to get Bun workspaces working with changesets. I wrote a blog post here on my website on how to do this: https://ianm.com/posts/2025-08-18-setting-up-changesets-with-bun-workspaces |
|
+1 cannot get this to work with yarn |
|
This is going to be superseded by #2097 |
fixes #598
fixes #432
fixes #813
fixes #1454
(maybe #1411)
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)