[CARE-1652] Switch from [`npm` and `nx`] to [`pnpm` and `turbo`] #293

MohammadxAli · 2023-05-18T22:31:27Z

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 and npm 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

See: sindresorhus/ama#479 (comment)

riffbyte

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.

riffbyte · 2023-05-19T11:36:17Z

.github/workflows/playwright.shared.yml

@@ -21,19 +21,26 @@ jobs:
    runs-on: ubuntu-latest

    steps:
-    - uses: actions/checkout@v3
-    - uses: actions/setup-node@v3
+    - name: Check out Git repository


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

Yes, I was just testing it with Bea to ensure everything is working, I have this in mind.

riffbyte · 2023-05-19T11:38:16Z

.gitignore

+yarn.lock
+package-lock.json
+pnpm-lock.yaml


pnpm-lock should not be ignored 🤔

I had the same thing in mind, but check this out:

prezly/slate#412 (comment)

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.

Oh, that's interesting. It would be nice to add this link in a comment here to prevent confusion in the future.

riffbyte · 2023-05-19T11:40:10Z

lerna.json

  "useWorkspaces": true,
-  "version": "5.2.3"
+  "version": "0.83.1"


I guess you missed this one when copying the config from elsewhere ;)
It should refer to the latest published package version (5.2.3)

That was a sleepy commit 🙂 Thanks

riffbyte · 2023-05-19T11:40:58Z

README.md

-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:


Suggested change

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:

riffbyte · 2023-05-19T11:41:54Z

README.md

-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).


This line is no longer relevant, since the link contains NX docs.

We still use Lerna for publishing though, I guess it makes sense to update the wording to reflect that, right?

riffbyte · 2023-05-19T11:48:42Z

package.json

+    "lint": "pnpm --recursive lint",
+    "lint:fix": "pnpm --recursive lint --fix",


Shouldn't this be ran with turbo?

riffbyte · 2023-05-19T11:49:36Z

package.json

+    "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",


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.

riffbyte · 2023-05-19T11:54:02Z

package.json

+    "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",


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.

package.json

riffbyte · 2023-05-19T11:55:33Z

.pnpmfile.cjs

@@ -0,0 +1,36 @@
+const OMIT_DEPENDENCIES = ['@types/react', '@types/react-dom', 'react', 'react-dom', 'next'];
+
+function readPackage(pkg, context) {


Could you explain why is this needed? 🤔

MohammadxAli · 2023-05-19T13:41:43Z

As we discussed with Oleg over Slack, I will split this PR into 2 separate PRs:

One for switching from NPM to PNPM
One for switching from NX to Turborepo ([CARE-1659] Switch from NX to Turborepo #296)

So that it's easier to review.

MohammadxAli added 9 commits May 18, 2023 21:16

Delete package-lock.json file

b6c6f42

Ignore yarn & npm lock files

93b68f0

Add build and coverage folders to .prettierignore

07f4866

Switch from npm to pnpm

2f6271c

Update github actions to use pnpm

d189fb7

Delete pnpm's lock file

c6ecd15

Ignore the lock file

127f676

See: sindresorhus/ama#479 (comment)

Setup dependency omit script

06a0a4e

Switch from nx to turbo

a48ea22

MohammadxAli added the refactor Not a bugfix nor a feature label May 18, 2023

MohammadxAli self-assigned this May 18, 2023

MohammadxAli added 2 commits May 19, 2023 02:05

Add -w flag to force installing sdk in workspace root

1e0424f

Define workspaces in the root package.json for lerna

8f485f5

MohammadxAli mentioned this pull request May 19, 2023

[CARE-1673] Switch from npm to pnpm prezly/theme-nextjs-bea#694

Closed

MohammadxAli marked this pull request as ready for review May 19, 2023 07:08

MohammadxAli requested review from e1himself and riffbyte May 19, 2023 07:14

MohammadxAli changed the title ~~Switch from [npm and nx] to [pnpm and turbo]~~ [CARE-1652] Switch from [npm and nx] to [pnpm and turbo] May 19, 2023

MohammadxAli added 5 commits May 19, 2023 11:36

Drop nx config file as it no longer needed

e5e6565

Fix typecheck and check scripts to use turbo

ce9204a

Update scripts section in readme to match the new structure

abc58be

Remove @playwright/test and @prezly/sdk from peer deps

05df994

Fix clean script

d9bbfe2

riffbyte suggested changes May 19, 2023

View reviewed changes

MohammadxAli added 2 commits May 19, 2023 16:01

Fix version regression

ed3354d

Add a link to comment where explains ignoring lock files

b4d083d

MohammadxAli marked this pull request as draft May 19, 2023 13:25

MohammadxAli requested review from riffbyte and removed request for e1himself and riffbyte May 19, 2023 13:25

MohammadxAli closed this May 19, 2023

MohammadxAli deleted the switch-to-pnpm branch May 24, 2023 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CARE-1652] Switch from [`npm` and `nx`] to [`pnpm` and `turbo`] #293

[CARE-1652] Switch from [`npm` and `nx`] to [`pnpm` and `turbo`] #293

MohammadxAli commented May 18, 2023 •

edited

Loading

riffbyte left a comment

riffbyte May 19, 2023

MohammadxAli May 19, 2023

riffbyte May 19, 2023

MohammadxAli May 19, 2023

riffbyte May 19, 2023

riffbyte May 19, 2023

MohammadxAli May 19, 2023

riffbyte May 19, 2023

riffbyte May 19, 2023

MohammadxAli May 19, 2023

riffbyte May 19, 2023

riffbyte May 19, 2023

riffbyte May 19, 2023

riffbyte May 19, 2023

MohammadxAli commented May 19, 2023 •

edited

Loading

	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:

		"lint": "pnpm --recursive lint",
		"lint:fix": "pnpm --recursive lint --fix",

		@@ -0,0 +1,36 @@
		const OMIT_DEPENDENCIES = ['@types/react', '@types/react-dom', 'react', 'react-dom', 'next'];

		function readPackage(pkg, context) {

[CARE-1652] Switch from [npm and nx] to [pnpm and turbo] #293

[CARE-1652] Switch from [npm and nx] to [pnpm and turbo] #293

Conversation

MohammadxAli commented May 18, 2023 • edited Loading

riffbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohammadxAli commented May 19, 2023 • edited Loading

[CARE-1652] Switch from [`npm` and `nx`] to [`pnpm` and `turbo`] #293

[CARE-1652] Switch from [`npm` and `nx`] to [`pnpm` and `turbo`] #293

MohammadxAli commented May 18, 2023 •

edited

Loading

MohammadxAli commented May 19, 2023 •

edited

Loading