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

NX Workspaces Support #165

Merged
merged 4 commits into from Jan 8, 2022
Merged

NX Workspaces Support #165

merged 4 commits into from Jan 8, 2022

Conversation

nishit-g
Copy link
Contributor

@nishit-g nishit-g commented Jan 8, 2022

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    feature: NX workspaces support [Tracking] NX workspaces support #164

  • What is the current behavior? (You can also link to an open issue here)
    lerna builds the packages

  • What is the new behavior (if this is a feature change)?
    NX builds the all the packages

  • Other information:
    Please test to confirm if commitizen is working or not as the "config" key from different packages/libs/providers have been moved to .czrc file in their respective directory.

@gitpod-io
Copy link

gitpod-io bot commented Jan 8, 2022

@vercel
Copy link

vercel bot commented Jan 8, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/dimagrossman/docs/JAeHZ7B1QmLaHh8Xww6fkxhfDLh4
✅ Preview: Canceled

@@ -69,7 +70,10 @@
"lint-staged": "^10.2.2",
"monorepo-run": "git+https://github.com/scopsy/monorepo-run.git",
"prettier": "^2.0.5",
"rimraf": "^3.0.2"
"rimraf": "^3.0.2",
"@nrwl/workspace": "latest",
Copy link
Contributor

Choose a reason for hiding this comment

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

@nishit-g is that the nx convention? Maybe it's better if we will pin this to a specific version?

@@ -2,6 +2,7 @@
"name": "notifirehq",
"private": true,
"scripts": {
"nx:build": "pnpm nx run-many --target=build --all",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can change the build we currently have instead of adding a specific command for that. Another not really related task, is that maybe we can add pnpm as a devDependency to the package.json file so we don't need to prefix with pnpm inside the scripts section in package.json

Comment on lines -70 to -74
"config": {
"commitizen": {
"path": "cz-conventional-changelog"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch on updating all the packages with the new .czrc! 😍

@scopsy scopsy changed the title DRAFT: NX Workspaces Support NX Workspaces Support Jan 8, 2022
@scopsy
Copy link
Contributor

scopsy commented Jan 8, 2022

Incredible job @nishit-g ! Looks very clean and works perfectly on my tests 🎉

@scopsy
Copy link
Contributor

scopsy commented Jan 8, 2022

@nishit-g I'll merge this for now, really want to start playing with it. So I will fix the small comments I mentioned in the review.

Incredible work and super exciting 🤩

@scopsy scopsy merged commit 708be02 into novuhq:master Jan 8, 2022
@nishit-g
Copy link
Contributor Author

nishit-g commented Jan 8, 2022

@scopsy was just about to push the new changes!

@scopsy scopsy mentioned this pull request Jan 8, 2022
5 tasks
@scopsy
Copy link
Contributor

scopsy commented Jan 8, 2022

@nishit-g Oh sorry! Push them, i'll merge again. Create a new PR for it from the same branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants