Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: sort pre/post scripts with colon together #330

Merged
merged 2 commits into from
Jan 13, 2025
Merged

feat: sort pre/post scripts with colon together #330

merged 2 commits into from
Jan 13, 2025

Conversation

chouchouji
Copy link
Contributor

@chouchouji chouchouji commented Jan 11, 2025 •
edited
Loading

Issue

close #307

Test

image

@chouchouji
Copy link
Contributor Author

@keithamus I create a pr, please review it if you have time.

keithamus
keithamus reviewed Jan 11, 2025
View reviewed changes
Copy link
Owner

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks reasonable, but it would be great to see some tests that exercise this functionality, to ensure we don't regress.

@chouchouji
Copy link
Contributor Author

The code looks reasonable, but it would be great to see some tests that exercise this functionality, to ensure we don't regress.

Thanks for your review. I have added test case.

keithamus
keithamus approved these changes Jan 13, 2025
View reviewed changes
Copy link
Owner

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @chouchouji! Let's ship it!

@keithamus keithamus merged commit eb49298 into keithamus:main Jan 13, 2025
4 checks passed
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 2.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@chouchouji chouchouji mentioned this pull request Jan 14, 2025
Closed
@chouchouji
Copy link
Contributor Author

Thanks for your work on this @chouchouji! Let's ship it!

Thanks!

@rfalke-rtl rfalke-rtl mentioned this pull request Jan 21, 2025
Open
@jfairley
Copy link
Contributor

I would've never expected this to be the sort order. What is the significance of the numbered scripts, or rather what recognizes them (npm core, npm-run-all2, something else)? I'm not familiar with this pattern.

This change messes up patterns I use often with alphas after the colon like:

Screenshot 2025-01-22 at 4 59 35 PM

👎🏼

prestart:prod should be aligned with start:prod, not 3 lines away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithamus keithamus keithamus approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sort pre/post scripts with colon together
3 participants
@chouchouji @jfairley @keithamus