Skip to content

chore: use pnpm instead of npm#1567

Merged
nperez0111 merged 63 commits intomainfrom
pnpm
Apr 4, 2025
Merged

chore: use pnpm instead of npm#1567
nperez0111 merged 63 commits intomainfrom
pnpm

Conversation

@nperez0111
Copy link
Copy Markdown
Contributor

@nperez0111 nperez0111 commented Mar 26, 2025

This PR introduces using pnpm instead of npm, pnpm is much better suited for:

  • monorepos
  • speed
  • running commands across packages (taking over part of lerna's role)

It also completely replaces lerna in favor of NX (which lerna was already based on). This gives us one tool to:

  • run commands and cache their results
  • release packages

This PR also renames several PNPM scripts, to be more standard. The goal should be for PNPM scripts to already have the most commonly used tasks that you might need within the repo. It should be possible to do everything you need from the root of the repo.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote 🛑 Canceled (Inspect) Apr 4, 2025 3:11pm
blocknote-website ✅ Ready (Inspect) Visit Preview Apr 4, 2025 3:11pm

Copy link
Copy Markdown
Collaborator

@matthewlipski matthewlipski left a comment

Choose a reason for hiding this comment

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

Great changes, overall experience with nx + pnpm is much nicer than lerna + npm. Just left a few comments, mainly commands that I've encountered some issues with.

Comment thread packages/react/src/hooks/useUIElementPositioning.ts Outdated
Comment thread tests/playwright.config.ts Outdated
// command: process.env.DEVSERVER ? "npm run start" : "npm run start:built",
// port: 3000,
// },
webServer: {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unless this is commented out, I'm getting errors trying to run test:updateSnaps - seems like pnpm is not recognized in the Docker container? @YousefED can you see if this is the case for you? Went over it on call with Nick and we couldn't find a solution, so would be at least good to know if it's just a problem on my side.

Comment thread package.json
Comment thread package.json
Comment thread package.json
@nperez0111 nperez0111 requested a review from YousefED April 1, 2025 14:02
@nperez0111
Copy link
Copy Markdown
Contributor Author

This is ready for review now @YousefED

Copy link
Copy Markdown
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

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

awesome!! looking forward to using this

Comment thread .github/workflows/build.yml
Comment thread .github/workflows/build.yml Outdated
Comment on lines +64 to +66
- name: Soft release
id: soft-release
run: pnpx pkg-pr-new publish './packages/*' # --compact enable compact after release
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cool! when do you use it for testing?

@@ -0,0 +1,61 @@
# ./.github/workflows/publish.yml
name: Publish
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

awesome! very curious about this!

Comment thread .npmrc Outdated
@@ -0,0 +1,4 @@
link-workspace-packages=deep
prefer-workspace-packages=true
public-hoist-pattern[]=prosemirror-*
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why do we need this?

Also; maybe cleaner to use pnpm-workspace.yaml (I could find the dashed options because the pnpm docs describe the yaml options which are camelCase :/ )

@@ -1,4 +1,4 @@
import glob from "fast-glob";
import { globSync } from "tinyglobby";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what was the reason for this? curious

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was trying to figure out why the gen step went from 2 seconds to 45 seconds. Turns out that we were globbing through node_modules that pnpm put in each directory.
I thought that the globbing library may have been an issue with the performance. So I switched out the package for a lighter weight one, but I then found it going through the node_modules and had no way of filtering so I had to rewrite it. So, I kept this lib

} from "@floating-ui/react";
import { useEffect, useMemo } from "react";

type UIElementPosition = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm gessing a ts change or sth that made this necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, otherwise:

 boolean,
YousefED1 day ago
I'm gessing a ts change or sth that made this necessary?

The inferred type of 'useUIElementPositioning' cannot be named without a reference to '.pnpm/@floating-ui+react-dom@2.1.2_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@floating-ui/react-dom'. This is likely not portable. A type annotation is necessary.

Triggered because I regenerated the node_modules so it upgraded TS

<Relationship Id="FAKE-ID" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink" Target="" TargetMode="External"/>
<Relationship Id="FAKE-ID" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink" Target="https://www.blocknotejs.org" TargetMode="External"/>
<Relationship Id="FAKE-ID" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/image" Target="media/e1037269513fbdc121e06bb1bc7cf37b9045c8fc.gif"/>
<Relationship Id="FAKE-ID" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/fontTable" Target="fontTable.xml"/>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

triggered by an upgrade?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, upgrade of a patch version

"vite-plugin-eslint": "^1.8.1",
"vitest": "^2.0.3"
"vitest": "^2.0.3",
"react": "^19",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should this be ^18?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I put 19 to test against that, but 19 is stable now so it shouldn't matter which

import { pdfStyleMappingForDefaultSchema } from "./styles.js";

export const pdfDefaultSchemaMappings = {
export const pdfDefaultSchemaMappings: {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

weird that this was needed, typescript issue?

Comment thread pnpm-workspace.yaml
- "examples/*/*"
- "playground"
- "docs"
- "shared"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@nperez0111 I'm not sure shared should be part of the workspace.

The way it is now, shared contains some code (utility functions) that's shared by different packages (mainly, the exporters). However, it's not bundled in it's own package. Rather, the functions are just included in the consuming packages

a) should we remove it here?
b) I tried to remove it, but then got an error in one of the exporters (TS errors in toMatchBinaryFileSnapshot)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The workspace defines where to install packages from. By default, pnpm installs packages "scoped" i.e. a project can only resolve the packages that it declares.

So, shared has dependency on image-meta (imageUtil) & @types/node (binarySnapshotUtil). So when you remove it from the workspace, it can no longer find those packages since they exist in the project they are used in, not in it's source path.

There are two ways to resolve this. Make shared into a workspace package (leveraging it only as an project, not a package), or install it's deps like I did.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or, remove the shared project altogether & just keep the files into their respective projects, which I'm most partial to since it keeps things simple.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Or, remove the shared project altogether & just keep the files into their respective projects, which I'm most partial to since it keeps things simple.

I think for code duplication that's not ideal. I think specifically for test utilities we should actually move a lot more to the shared package (test cases and utilities that are shared across packages) - unless you have a better solution for this.

There are two ways to resolve this. Make shared into a workspace package (leveraging it only as an project, not a package), or install it's deps like I did.

I'm not sure I understand the difference between the two. Currently it's a workspace package right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think for code duplication that's not ideal. I think specifically for test utilities we should actually move a lot more to the shared package (test cases and utilities that are shared across packages) - unless you have a better solution for this.

I think test utilities should be in/reusing the existing test package:

"name": "@blocknote/tests",
it's a package, so it is perfectly valid to import/export things from it.

So, the only one that I think is valid to live here is the image util, and even then, I don't see why this can't be either duplicated or exported from @blocknote/core (ideally their would actually be a @blocknote/core/exporter sub package for this reason)

I'm not sure I understand the difference between the two. Currently it's a workspace package right?

Sorry, I could've explained that better. The difference is:

  1. workspace package: @blocknote/shared as a devDep to whatever uses it (so linking via pnpm workspaces)
  2. tricking vite to resolve it like we are doing now:
    ? ({
    "@shared": path.resolve(__dirname, "../../shared/"),
    } as Record<string, string>)

In either of these cases, you still need anything shared depends to be installed to this directory (whether you consider it a package or not).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks.

For (1), wouldn't it require us to publish that package to npm? (if we use functions from it in non-testing code)?

Copy link
Copy Markdown
Contributor Author

@nperez0111 nperez0111 Apr 7, 2025

Choose a reason for hiding this comment

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

Nope, you can set it up where @blocknote/shared is declared as not "external" (i.e. vite/rollup should bundle it into the package that you are trying to build like docx-exporter). So, at build time the import would be resolved correctly within the repo & the consumer would not need (or even know about) that package.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah makes sense. I suppose that's cleaner than what we currently have, because dependencies are better managed right?

Should we go for that approach? Also happy to discuss whether the package is needed at all - I think easier to go over that on a call

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.

3 participants