Refactor create-astro#6082
Conversation
🦋 Changeset detectedLatest commit: 7469554 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 |
|
!preview refactor |
|
|
Like how the refactor allows better testing, love all of the new tests that are coming out of this. Not sure I feel about using vitest here. Putting aside whether it's appropriate in this context (given that it compiles code differently than in prod), having multiple test libraries in the same repo does make it harder to work with. I'm open to changing test libraries, but I don't think a refactor of one package is the right place to make that decision. I see that some of the tests are relying on vitest's mocking capabilities, was that the reason to switch? I would think this code could be changed to not need the mocking. For example there is a test that checks for process exit. Can this be refactored so that process.exit() is not called directly in the code, but instead you pass it a function (just as an example) for when the process should exit. That way you can test without needing to mock. |
yanthomasdev
left a comment
There was a problem hiding this comment.
Great work @natemoo-re, docs LGTM! I do have a small nitpick regarding a prompt message that can be dismissed if intentional 🙌
Yes, I was using |
|
@matthewp tests refactored to stick with Everything is accessed through a shared |
matthewp
left a comment
There was a problem hiding this comment.
Code looks good, love all of the new testing and how easy that seems to be.
Adding Sarah and Fred as reviewers since they have context about the UI changes.
|
@natemoo-re can you share a screen recording of the whole thing? I'll push to make sure this convo doesn't get too bike-sheddy if that's a concern, but I think it's still an important part of evaluating this PR. I'll save my review to only look at that! |
sarah11918
left a comment
There was a problem hiding this comment.
Approve of the new TypeScript messages! Looking forward to dealing with this less in docs! 😅
|
Looks great! I may still push to tweak in the future (I'd still love to find a way to remove that "strictness" question) but this is absolutely an improvement and I am HERE for it. |
|
!preview refactor |
bholmesdev
left a comment
There was a problem hiding this comment.
Creating an astronomical improvement 👏
|
Princesseuh
left a comment
There was a problem hiding this comment.
I'll fight with Fred, scream and throw tantrums for the "strictness" question to be kept forever, but just wanted to add my approval to the wonderful work you've done here Nate!
Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
d5fe9d8 to
d946da9
Compare
Changes
create-astro, with more features and much better CLI flagscreate-astro-ts-nope.mp4
create-astro-ts-yep.mp4
Testing
create-astrohad very poor test coverage previously. We were testing the CLI output directly, which was flaky and led to most tests being disabled.create-astrocompletely so thatpromptand other internals can be mocked and the logic of each step can be tested independentlyDocs
Docs changes not needed