Simplify auth state handling - #2065
Conversation
🦋 Changeset detectedLatest commit: ffb133f 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 |
a94ac0d to
50a1d8a
Compare
5363c57 to
afc609c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2065 +/- ##
==========================================
+ Coverage 85.52% 87.19% +1.67%
==========================================
Files 66 71 +5
Lines 2535 2523 -12
Branches 700 676 -24
==========================================
+ Hits 2168 2200 +32
+ Misses 337 299 -38
+ Partials 30 24 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
50a1d8a to
f508136
Compare
b67ce93 to
45a76cd
Compare
| // in the delegated mode we don't need the json output | ||
| // as we won't be handling the auth errors |
There was a problem hiding this comment.
should we split the two modes into functions? i think it would make the flow easier to follow
e.g. publishInteractive, publishNonInteractive?
There was a problem hiding this comment.
it's certainly complicated - but I just don't want to refactor this all too much right now
| target = packagesByName.get(release.name)!.dir; | ||
| } else { | ||
| const pkg = packagesByName.get(release.name)!; | ||
| // npm, yarn classic and berry don't support `publishConfig.directory` natively. |
There was a problem hiding this comment.
I'd much prefer we consider removing support of this as this feels non-standard, but we can discuss this later.
There was a problem hiding this comment.
I looked into this. There are situations and tools that require supporting this. If we'd drop this, we would just break the compatibility with them and I don't think it's particularly worth it.
| // However, there are still 2 reasons why we prefer to set it to the root: | ||
| // 1. it's a consistent approach that works across package managers and it's also a good location when publishing packed artifacts | ||
| // 2. it's important not to call npm from a directory that is an actual workspace (as per the workspaces configuration) and not from, for example, publishConfig.directory | ||
| // because npm only resolves to the root's .npmrc for actual workspaces and not for arbitrary subdirectories of the root. | ||
| cwd: packages.rootDir, |
There was a problem hiding this comment.
I would really prefer we call in the workspace directory instead. I don't think no1 is really important as it's a poor implementation from old npm versions. no2 I don't think is an issue if we use pkg.dir here?
Also typo?
| // However, there are still 2 reasons why we prefer to set it to the root: | |
| // 1. it's a consistent approach that works across package managers and it's also a good location when publishing packed artifacts | |
| // 2. it's important not to call npm from a directory that is an actual workspace (as per the workspaces configuration) and not from, for example, publishConfig.directory | |
| // because npm only resolves to the root's .npmrc for actual workspaces and not for arbitrary subdirectories of the root. | |
| cwd: packages.rootDir, | |
| // However, there are still 2 reasons why we prefer to set it to the root: | |
| // 1. it's a consistent approach that works across package managers and it's also a good location when publishing packed artifacts | |
| // 2. it's important to call npm from a directory that is an actual workspace (as per the workspaces configuration) and not from, for example, publishConfig.directory | |
| // because npm only resolves to the root's .npmrc for actual workspaces and not for arbitrary subdirectories of the root. | |
| cwd: packages.rootDir, |
There was a problem hiding this comment.
I juggled this so we call it with cwd: pkg.dir
f508136 to
28857c1
Compare
45a76cd to
7cdb754
Compare
7cdb754 to
3b9c86b
Compare
beeequeue
left a comment
There was a problem hiding this comment.
this is still way too complicated for my liking...
i would love to extract all the logic into PM-specific utilities to help organize it better, in a later PR
No description provided.