-
Notifications
You must be signed in to change notification settings - Fork 2
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
[CARE-1652] Switch from [npm
and nx
] to [pnpm
and turbo
]
#293
Conversation
npm
and nx
] to [pnpm
and turbo
]npm
and nx
] to [pnpm
and turbo
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can't approve it in this state.
There is a lot happening in this PR, and I don't quite get why you needed to change the whole build pipeline just for enabling PNPM linking. I thought NX does support PNPM 🤔
I believe we should be a bit more thoughtful on things like that. I wouldn't consider this new setup more convenient than releasing preview versions.
@@ -21,19 +21,26 @@ jobs: | |||
runs-on: ubuntu-latest | |||
|
|||
steps: | |||
- uses: actions/checkout@v3 | |||
- uses: actions/setup-node@v3 | |||
- name: Check out Git repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that as soon as you merge this PR, all themes that are not yet using PNPM will have the Playwright workflow broken. So likely better to merge the themes PRs before this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was just testing it with Bea to ensure everything is working, I have this in mind.
yarn.lock | ||
package-lock.json | ||
pnpm-lock.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm-lock should not be ignored 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thing in mind, but check this out:
And another thing is when we're omitting some dependencies the lock file changes which results on different lock files and it's hard to maintain that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's interesting. It would be nice to add this link in a comment here to prevent confusion in the future.
lerna.json
Outdated
"useWorkspaces": true, | ||
"version": "5.2.3" | ||
"version": "0.83.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you missed this one when copying the config from elsewhere ;)
It should refer to the latest published package version (5.2.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a sleepy commit 🙂 Thanks
After pulling the repo, run `npm i` to install the dependencies. This will install the root dependencies, as well as dependencies for individual packages. | ||
After pulling the repo, run `pnpm install` to install the dependencies. This will install the root dependencies, as well as dependencies for individual packages. | ||
|
||
If you don't pnpm installed, install it with the following command globally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't pnpm installed, install it with the following command globally: | |
If you don't have PNPM installed, install it with the following command globally: |
The repo is designed to run all of the scripts from the root directory with the help of [Lerna] and [Nx]. | ||
This ensures that all tasks are aware of dependencies between packages. Nx also provides caching, which speeds up the repeated tasks. | ||
The repo is designed to run all of the scripts from the root directory with the help of [Lerna] and [Turbo]. | ||
This ensures that all tasks are aware of dependencies between packages. Turbo also provides caching, which speeds up the repeated tasks. | ||
You can learn more about task pipeline configuration from [Lerna docs](https://lerna.js.org/docs/concepts/task-pipeline-configuration). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is no longer relevant, since the link contains NX docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still use Lerna for publishing though, I guess it makes sense to update the wording to reflect that, right?
"lint": "pnpm --recursive lint", | ||
"lint:fix": "pnpm --recursive lint --fix", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be ran with turbo
?
"prettier:fix": "pnpm prettier --write --no-list-different", | ||
"test": "turbo run test", | ||
"typecheck": "turbo run typecheck", | ||
"check": "turbo run lint && turbo run typecheck && turbo run test", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to run multiple tasks at once with turbo
? The cool thing about NX was that some tasks could be run in parallel if they're not dependent on other tasks.
"release": "pnpm release:prepare && pnpm release:publish", | ||
"release:preview": "pnpm release:prepare && pnpm release:publish:preview", | ||
"release:build": "pnpm build --force", | ||
"release:prepare": "pnpm clean && pnpm install && pnpm release:build && pnpm check", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This differs massively from the current script.
- Scripts are not using
turbo
, so there's no caching. Judging by other scripts, even if you change one thing in a single package, everything will be re-built, which is not optimal. - Cleaning the build artifacts is already done in each package with the
prebuild
scripts. I believe it should stay that way, as each package could use a different build process (and produce different artifacts), so it's best to let the package scripts control the cleaning. - Is cleaning and installing dependencies again prior to build really needed? I found that it was an unnecessary step in most of our packages.
@@ -0,0 +1,36 @@ | |||
const OMIT_DEPENDENCIES = ['@types/react', '@types/react-dom', 'react', 'react-dom', 'next']; | |||
|
|||
function readPackage(pkg, context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why is this needed? 🤔
As we discussed with Oleg over Slack, I will split this PR into 2 separate PRs:
So that it's easier to review. |
I had a very hard time linking the package to a theme for developing purposes, for 2 days I tried anything but couldn't get it to work unless I switched to
pnpm
and I was able to make it work with pnpm workspaces.I tried both of
yalc
andnpm link
methods to link the package but in the end I got the famous wrong hooks call error.Unfortunately, I was blocked by this and couldn't move CARE-1572 forward since I can't really test/develop locally effectively.
Pretty much everything in this PR is inspired by the current https://github.com/prezly/slate setup
Instructions on how to attach theme-kit-js to a theme are explained at: prezly/theme-nextjs-bea#694