Conversation
🦋 Changeset detectedLatest commit: 26135a6 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 💥 An error occurred when fetching the changed packages and changesets in this PR |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #2009 +/- ##
==========================================
- Coverage 80.94% 80.88% -0.07%
==========================================
Files 54 53 -1
Lines 2362 2302 -60
Branches 722 710 -12
==========================================
- Hits 1912 1862 -50
+ Misses 408 389 -19
- Partials 42 51 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Andarist
left a comment
Andarist
left a comment
There was a problem hiding this comment.
I have not finished reviewing yet, but this one definitely feels blocking
| const cwd = options?.cwd ?? process.cwd(); | ||
|
|
||
| const packages = await getPackages(cwd); | ||
| await ensureChangesetFolder(packages.rootDir); |
There was a problem hiding this comment.
basically all commands (but init) need this - maybe instead of doing ensureChangesetFolder it would be good to push this into createCommand factory? that could accept an option to disable this requirement (just to support init).
The cwd + getPackages resolution could also be shoved there to be shared across all commands (those are actually resolved by all of them)
There was a problem hiding this comment.
Similarly, the config is being resolved for all of the commands... minus init (duh) and 🥁 pre.
There was a problem hiding this comment.
Maybe we can make a resolveCommandContext(cwd, { ... }) function that returns the cwd, packages, config etc? I think it's nice if we can see the execution step-by-step instead of an abstraction.
But also I think the current step is fine for granular control, and repetition is sometimes fine, but it's not a strong pattern I'll insist on.
There was a problem hiding this comment.
yeah, such heloer would work too 👍
There was a problem hiding this comment.
it's what i did for my cleye and gunshi implementations as well
There was a problem hiding this comment.
I tried to implement this but in the end I've written more code for the function than it's worth. I think we should stick to what we have now and revisit again once we get the changesets config refactor in and the rootDir sorted out so we get a better picture of the abstraction.
| await getVersionableChangedPackages(config, { | ||
| cwd, | ||
| ref: since, | ||
| cwd: packages.rootDir, |
There was a problem hiding this comment.
I wooooonder... what do we really need cwd for? Perhaps we should just always default to packages.rootDir? cwd is useful for the initial resolution of packages but I wonder if it's useful for anything else.
Just food for thought here, we certainly don't have to touch this in this PR
There was a problem hiding this comment.
It might have not been clear what I was alluding to here, so let me clarify - perhaps we should rename things to use rootDir name over cwd so it's more apparent what's that value and what's its origin. And pass packages.rootDir to those few places in which we actually still use the input cwd
There was a problem hiding this comment.
Yeah I think we need to follow-up making cwd and rootDir separation clearer. It's a bit tricky for some @changesets/* packages because they're sometimes used directly and some people may expect those functions to auto-resolve to the root-dir as well. We should revisit for sure and decide how to handle this better.
| if (options.ignore) { | ||
| options.ignore = Array.isArray(options.ignore) | ||
| ? options.ignore | ||
| : [options.ignore]; | ||
| } |
There was a problem hiding this comment.
note to myself: it would be nice to report the original issue related to this workaround to cac
There was a problem hiding this comment.
I ended up putting another fix where if a flag is passed multiple times (the option becomes an array), we pick the last value. And put these inside normalizeOptions. So even if this is fixed upstream, it might be better to stick with our little utility.
Note the flag array issue also happens in main with plain mri usage.
There was a problem hiding this comment.
It would be nice if we could get a test for this file that would test the discovered ignore thing and perhaps support for mixed snapshot boolean/string support. But it's not super important
There was a problem hiding this comment.
Agree, regression tests are awesome 😎
There was a problem hiding this comment.
I wrote some tests initially, exporting the cli which exposes some things to test, but since a lot of the handlers are thin and we should be trusting cac's behaviour, the tests ended up not useful I think. Probably useful now with some quirks we found from cac though.
|
I'll merge this in now, but I'll follow-up on the tests later as I'm thinking of moving some things around and I don't want to touch to much to review again. |
3 participants
Main changes:
Refactor each
src/commands/*to independently resolve the required monorepo root and config etc. This makes calling the command function simple.(This changes the command function signature. Let me know if you feel differently of how this is refactored. I went back-and-forth between
cwdbeing its own param vs inside the options object.)src/run.tsremoved. Move the CLI handling all tosrc/index.ts.src/run.tests.tsremoved. Move to respective commands tests.