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

First public release #1

Merged
merged 7 commits into from
Nov 15, 2024
Merged

First public release #1

merged 7 commits into from
Nov 15, 2024

Conversation

mangs
Copy link
Contributor

@mangs mangs commented Nov 14, 2024 •
edited
Loading

Pull Request Checklist

  • Readme and changelog updates were made reflecting this PR's changes

Changes Included

  • First public release: version 1.0.0

mangs
mangs commented Nov 14, 2024
View reviewed changes
Copy link
Contributor Author

@mangs mangs left a comment

Choose a reason for hiding this comment

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

Some notes for context:

.github/workflows/pullRequestWorkflow.yml
Comment on lines +16 to +21
# - run: bun run check:package-version
# env:
# GITHUB_API_URL: ${{ env.GITHUB_API_URL }}
# GITHUB_REF_NAME: ${{ env.GITHUB_REF_NAME }}
# GITHUB_REPOSITORY: ${{ env.GITHUB_REPOSITORY }}
# GITHUB_TOKEN: ${{ github.token }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a convenience check during CI that will fail if the PR doesn't increment the package.json version and match the latest version in CHANGELOG.md. So instead of needing to remember to do it manually and having a reminder in the pull request template, it's now automated. However, since this is the first version being published, it doesn't work so needs to be disabled until the 2nd PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a follow-up PR to turn this on

Copy link

Choose a reason for hiding this comment

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

Are we sure we want this?

Forcing package version update for all merges may sound nice initially, but then it means no change can be merged without a release, which isn't ideal as there are valid changes that don't require releases, such as documentation only changes, local development only changes (local scripts, dev dependencies update).

It also prevents the ability of batching updates (though for this I think, given we have a low flow of contributions, it's also ok to use release branches)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it very useful. Documentation or local development changes can be a patch release. Plenty of open source packages follow this philosophy. I see more upsides than downsides mainly because it makes the publishing process more foolproof and less error prone; in other words, you don't have to tell people what to do, it does it for you.

Copy link

Choose a reason for hiding this comment

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

Yeah I understand the benefits, I am personally not a big fan of libraries having releases for changes that don't affect end users, but definitely more an opinion than anything so all good.

lib/stylelintBaseConfig.json
@@ -0,0 +1,67 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up not making any changes to the config files. The main change was updating the dependency versions to latest.

package.json
"stylelint-config"
],
"engines": {
"node": "^18.12.0 || ^20.9.0 || >=22.11.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

somewhere along the line Stylelint 16 upgraded their baseline Node version to 18.12.0, so matching that here in addition to supporting all other current LTS versions

package.json
"@babbel/eslint-config": "2.1.1",
"@mangs/bun-utils": "2.33.2",
"eslint": "8.57.1",
"marked": "15.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

marked is necessary for the check:package-version script

@mangs mangs marked this pull request as ready for review November 14, 2024 23:44
@mangs mangs requested a review from jduthon November 14, 2024 23:44
jduthon
jduthon approved these changes Nov 15, 2024
View reviewed changes
Copy link

@jduthon jduthon left a comment

Choose a reason for hiding this comment

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

Left two small comments for future maintainability, but looks good already 👏

config/github/codeOwners Outdated
@@ -0,0 +1 @@
* @mangs @jduthon
Copy link

Choose a reason for hiding this comment

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

Interesting 🤔
Looks like I am missing some right I guess, I'll have a look 😅

I am thinking, ideally we would put our whole team here, so if one of us isn't around things aren't getting blocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I matched the ESLint list, but happy to add to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I noticed the error about your account which confused me but I didn't know how to resolve it. Any ideas?

Copy link

Choose a reason for hiding this comment

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

I think I noticed last time I am not part of a team for the Babbel org, but didn't take the time to fix it yet 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're part of the Pineapples team

.github/workflows/pullRequestWorkflow.yml
Comment on lines +16 to +21
# - run: bun run check:package-version
# env:
# GITHUB_API_URL: ${{ env.GITHUB_API_URL }}
# GITHUB_REF_NAME: ${{ env.GITHUB_REF_NAME }}
# GITHUB_REPOSITORY: ${{ env.GITHUB_REPOSITORY }}
# GITHUB_TOKEN: ${{ github.token }}
Copy link

Choose a reason for hiding this comment

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

Are we sure we want this?

Forcing package version update for all merges may sound nice initially, but then it means no change can be merged without a release, which isn't ideal as there are valid changes that don't require releases, such as documentation only changes, local development only changes (local scripts, dev dependencies update).

It also prevents the ability of batching updates (though for this I think, given we have a low flow of contributions, it's also ok to use release branches)

jduthon
jduthon approved these changes Nov 15, 2024
View reviewed changes
Copy link

@jduthon jduthon left a comment

Choose a reason for hiding this comment

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

🚀

@mangs mangs merged commit 5398498 into main Nov 15, 2024
1 check passed
@mangs mangs deleted the first-public-release branch November 15, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jduthon jduthon jduthon approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mangs @jduthon