🦋 Changeset detected

Latest commit: 0bbcc29

The changes in this PR will be included in the next version bump.

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

Codecov Report

❌ Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.47%. Comparing base (12f20ea) to head (0bbcc29).
⚠️ Report is 1 commits behind head on next.

@@            Coverage Diff             @@
##             next    #1655      +/-   ##
==========================================
- Coverage   82.52%   82.47%   -0.05%     
==========================================
  Files          53       53              
  Lines        2357     2357              
  Branches      697      697              
==========================================
- Hits         1945     1944       -1     
- Misses        370      371       +1     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TODO: Update this once main cuts a release, which then I'll update the next branch with main changes. And then later port over some changes from #1609 and #1069 which attempted to upgrade to v2 in main

Hi @bluwy any update on merging this?

I haven't finished the tasks I mentioned in my last comment, but I'm also unfortunately not interested in updating anymore given I had no reviews when it was ready. If anyone would like to help out, that'd be much appreciated.

This is a prerequisite to this work: #1872 . I'll land this soon to unblock this work here, see the discussion that led to this PR here: #1795 (comment)

@marcalexiei I know you were interested in this work. I just updated this PR after landing your work. There are known type errors here because the Changesets' Packages require a string literal for the tool (it assumes known tools) whereas the manypkg's Tool is now more generic and the tool type is typed as just a string.

Treating it as string is probably more future-proof and flexible... but it's also kinda more annoying. So dunno, I invite you to take a look at the current state of this PR and participae in the discussion.

@Andarist I'm not sure this is the most ergonomic, shouldn't npm monorepos be detected as fallback without the need for package-lock.json to exist?

The new requirement is more strict but I think it's ultimately more correct. We might want to differentiate further in the future between npm-managed workspaces and root-only repositories without lockfiles.

Unrelated but if I may give feedback on the new breaking APIs, I'm not a big fan of passing the tool object entirely from @manypkg/tools. It should've supported a simple string like before and a union of Tool object instead.

This was largely addressed by #1872 , WDYT?

Also, I'm not entirely clear for Packages type, why the returned rootPackage could be undefined while rootDir is not? rootPackage shouldn't be possible to be undefined from what I can tell (at least for default tools).

I thiiiink Rush might not support the root package in the same sense as other tools support it today. Its workspaces might be defined in its own config file or something. But honestly, I'd have to do a deeper research on this to answer this properly.

Also, I'm not sure if it was necessarily to introduce both dir (absolute) and relativeDir (relative) in the object. Feels redundant.

This was also addressed by #1872

I just updated this PR after landing your work. There are known type errors here because the Changesets' Packages require a string literal for the tool (it assumes known tools) whereas the manypkg's Tool is now more generic and the tool type is typed as just a string.
Treating it as string is probably more future-proof and flexible... but it's also kinda more annoying. So dunno, I invite you to take a look at the current state of this PR and participae in the discussion.

How about using a 'npm' | 'pnpm' | ... | (string & {}) union for Changesets’ Packages?
It seems like a reasonable compromise:

• we preserve type safety for known tool.type values
• we allow extensibility, and we avoid the need for type casting.

Would you like me to open a PR on this branch using this approach?

This was a simple fix - so I think it wasn't worth opening a different PR for this. I just pushed it out here.

Do you think it's worth keeping up with manypkg's compatibility or at this point it's just making things more complicated than they should be? I'm torn on this - but also, manypkg is also controlled by us so we can refactor it however we want to suit our needs. So ultimately, it's kinda a choice of:

• landing this PR and settling on this whole approach
• refactoring manypkg somehow
• creating @changesets/get-packages that would repeat most of what manypkg's package does already

Changesets is the main consumer followed by backstage. Not sure we need to worry too much about anyone else

$ npx github:Fuzzyma/e18e-tools @manypkg/get-packages -n 20 -U https://npm.devminer.xyz/registry -q -o md

This was a simple fix - so I think it wasn't worth opening a different PR for this. I just pushed it out here.

Perfect, thank you!

Do you think it's worth keeping up with manypkg's compatibility or at this point it's just making things more complicated than they should be? I'm torn on this - but also, manypkg is also controlled by us so we can refactor it however we want to suit our needs.

Personally, I’d lean toward option 1 (landing this PR and settling on this approach).

The main advantage is that moving to an object-based tool gives us forward compatibility. It creates space to evolve the API (e.g. adding new properties or methods) without forcing breaking changes later.

I’m not sure yet what we might need for tools like Rush, but this keeps that door open.

It also enables more tool-specific extensions in a type-safe way. For example:

type Tool =
  | { type: 'pnpm'; pnpmMethod: () => void }
  | { type: 'yarn'; yarnMethod: () => void }
  | { type: string | {} };

function isToolTypeKnown(x: Tool): x is Extract<Tool, { type: 'pnpm' | 'yarn' }> {
  return x.type === 'pnpm';
}

const tool = {} as Tool;

if (isToolTypeKnown(tool)) {
  if (tool.type === 'yarn') {
    tool.yarnMethod();
  } else {
    tool.pnpmMethod();
  }
}

If this direction works well in practice, we could then revisit whether @changesets/get-packages should evolve accordingly.

That sounds good to me. I'll wait a couple of days before landing this to give @bluwy the chance to comment too.

We still need to remember to update the bot. If you'd be interested in helping with that, I'd really appreciate it.

We still need to remember to update the bot. If you'd be interested in helping with that, I'd really appreciate it.

I can take a pass at this. I’ll open an issue in the bot repo with all the relevant references so we don’t lose track (I remember you already pointed out the parts that need updating).
Once this PR is merged, I’ll start working on it.

Sounds good?

Sounds good! thanks 🙏

That sounds good to me. I'll wait a couple of days before landing this to give @bluwy the chance to comment too.

I'll leave it to y'all to handle this as I don't have much time to follow-up on this lately.