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.

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

chore: Upgrade to pnpm 9, add conventional commit PR title linting #5598

Merged
merged 38 commits into from
May 20, 2024

Conversation

rifont
Copy link
Contributor

@rifont rifont commented May 18, 2024

What changed? Why was the change needed?

  • Upgrade to pnpm 9
    • replace all hardcoded versions of local packages with the workspace: directive, a requirement of PNPM v9. This has the benefit of ensuring we only depend on local packages, avoiding difficult-to-debug versioning mismatch issues.
  • add conventional commit PR title linting
  • update publish script to use nx release version, which updates the workspace: directive for packages to an actual version. These actual version changes should be reverted after publishing to NPM.

Screenshots

Add NX Version release to publish script (dry-run publish)
NB: we should look at upgrading to NX v19 to take advantage of their advanced release script capability.
image

Bad Conventional Commit types
image

Bad Conventional Commit Scope
image

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

@rifont rifont requested review from a team as code owners May 18, 2024 02:00
@rifont rifont requested review from scopsy and Cliftonz and removed request for a team May 18, 2024 02:00
Copy link

netlify bot commented May 18, 2024

Deploy Preview for dev-web-novu failed. Why did it fail? →

Name Link
🔨 Latest commit 64426d4
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/664b1bfed8a5b80008229e55

Copy link

netlify bot commented May 18, 2024

Deploy Preview for novu-design ready!

Name Link
🔨 Latest commit 64426d4
🔍 Latest deploy log https://app.netlify.com/sites/novu-design/deploys/664b1bfe0eee3b000818ba25
😎 Deploy Preview https://deploy-preview-5598--novu-design.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

.github/workflows/conventional-commit.yml Show resolved Hide resolved

```
${{ steps.lint_pr_title.outputs.error_message }}
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A friendly message added as a PR comment when the PR title is not conventional, alongside the actions needed to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This only affects the PR title? What about the squash commit message?

Copy link
Contributor Author

@rifont rifont May 19, 2024

Choose a reason for hiding this comment

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

It targets the PR title which in turn is used as the squash commit message (the highlighted part of this screenshot). The PR description is not targeted by this action.

This won't target unsquashed commit messages that may have been pushed. That job still lives with the Git-husky pre-commit linting hook. We'll need to enforce squash-merges on Git to ensure all commits on the tree are conventionally titled. Let's have that discussion separately.

.github/workflows/test.yml Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rifont rifont requested review from SokratisVidros and denis-kralj-novu and removed request for Cliftonz May 18, 2024 03:27

```
${{ steps.lint_pr_title.outputs.error_message }}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This only affects the PR title? What about the squash commit message?

package.json Outdated Show resolved Hide resolved
packages/cli-next/package.json Show resolved Hide resolved
Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

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

@rifont excellent work!

Couple of notes:

  1. If CM linting requires extra work at this point I'd address it in a separate PR so that this one remains focused on the PNPM upgrade. If it's ready, you can ignore the comment.
  2. It would be good to move @novu/application-generic from packages to libs. It's an 100% internal package that adds value only to Nest.js apps. I think that we might end up using the workspace: protocol for all the private packages inside libs folder but not for the public NPM packages inside the packages folder. The reason is that we publish NPM packages, we need to get rid of that workspace: protocol in package.json.

Point 2 can be addressed in a follow up PR.

"@novu/ee-echo-api": "^0.24.2",
"@novu/ee-shared-services": "^0.24.2",
"@novu/ee-translation": "^0.24.2"
"@novu/ee-auth": "workspace:",
Copy link
Contributor

Choose a reason for hiding this comment

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

@rifont This is gold! It was one of the big blockers to bundling all our source code in a single file in Nest.

@rifont
Copy link
Contributor Author

rifont commented May 20, 2024

  1. It would be good to move @novu/application-generic from packages to libs. It's an 100% internal package that adds value only to Nest.js apps. I think that we might end up using the workspace: protocol for all the private packages inside libs folder but not for the public NPM packages inside the packages folder. The reason is that we publish NPM packages, we need to get rid of that workspace: protocol in package.json.

@SokratisVidros I agree we should move @novu/application-generic to libs.

Regarding replacement of the workspace: to an actual version during the publishing process, the newly added nx release version part of the publish script handles this, so we can use workspace: protocol everywhere.

@rifont rifont merged commit 898ca79 into next May 20, 2024
19 of 24 checks passed
@rifont rifont deleted the upgrade-pnpm-v9 branch May 20, 2024 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants