Skip to content

Migrate to pnpm for package management #461

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

Merged
merged 42 commits into from
Apr 16, 2025
Merged

Migrate to pnpm for package management #461

merged 42 commits into from
Apr 16, 2025

Conversation

OskarDamkjaer
Copy link
Collaborator

@OskarDamkjaer OskarDamkjaer commented Apr 7, 2025

Moving to PNPM has a number of benefits over npm:

  • Packages can now only depend on their own package.json dependencies, this prevents:
    • Depending on internal packages (like neo4j-driver-core) which isn't meant for outside users
    • Depending on dependencies of our dependencies (for example 'glob')
    • Depending on other packages dependencies (vscode using the react installed by react-codemirror)
  • Faster package installs through offline caching
  • Update version number in one place for dependencies used by many, see cataglogs.
    • I only moved the driver so far, this let's us ensure that we don't have conflicting versions of packages (one driver version in poller another in vscode for example)
    • This makes sure that the dependencies get re-used so we don't ship multiple versions of the same dependency in our bundles
  • More ergonomic script running (you don't need to write run every time, just plain pnpm build works)
  • Consistent with the upx/console project and other front-end projects at neo4j
  • PNPM understands monorepos better than npm
    • We don't need turbo anymore to make sure packages get build in the right order. We can use --filter to build a given package and it's dependencies and --recursive to run the whole project.
      • Note that we still have caching from the typescript incremental: true
    • We can use the `workspace:*' for internal dependencies. This forces the use of source code when developing and when publishing it's replaced with the version used in the package.json. This means we can't accidentally depend packages from the NPM package registry instead of our monorepo.

I had problems getting our old Benchmark.js benchmarks to work properly, it's unmaintained and it seems to do some very odd import / exports. I found however that benchmarking is built in to vitest! So I got rid of that dependency all together 👍

For publishing, I've renamed the schema-poller package to a more generic name. I've also set up antrl4-c3 to be bundled without being part of the depencency list in package json. Here's how a new published version looks on verdaccio:
CleanShot 2025-04-11 at 14 42 20@2x

CleanShot 2025-04-11 at 14 40 20@2x

CleanShot 2025-04-11 at 14 42 13@2x

CleanShot 2025-04-11 at 14 42 07@2x

CleanShot 2025-04-11 at 14 41 59@2x

Copy link

changeset-bot bot commented Apr 7, 2025

🦋 Changeset detected

Latest commit: 8e886f9

The changes in this PR will be included in the next version bump.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@OskarDamkjaer OskarDamkjaer changed the title Pnpm move Migrate to pnpm for package management Apr 7, 2025
@OskarDamkjaer OskarDamkjaer marked this pull request as ready for review April 7, 2025 13:05
Comment on lines 22 to 30
- name: Setup pnpm
uses: pnpm/action-setup@v4

- name: Setup node.js
uses: actions/setup-node@v4
with:
node-version: 20
cache: 'pnpm'
registry-url: 'https://registry.npmjs.org'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we maybe extract this to one of our custom actions (e.g. setup-pnpm) given it's used in a few places?

See the ./.github/actions/setup-antlr4 for inspiration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also that way we'd just need to tweak the node version in one centralized place

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes for sure!

"lodash.debounce": "^4.0.8",
"neo4j-driver": "^5.3.0",
"neo4j-driver": "catalog:",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is so that we only set the version number in one place (in the pnpm-workspaces.yaml) file, there's some more info in the docs/ pr description

Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

LGTM!

Only comment is probably we want a clean script similar to the one in the upx repo to clean up everything including dependencies, because at some point I had a sticky error compiling which did not solve until I nuked down all of the node_modules folders.

│ tests/generated/WhiteboxParser.ts(841,15): error TS2339: Property 'getToken' does not exist on type 'Test3Context'.
│ tests/generated/WhiteboxParser.ts(844,15): error TS2339: Property 'getToken' does not exist on type 'Test3Context'.
│ tests/generated/WhiteboxParser.ts(847,19): error TS2339: Property 'getTokens' does not exist on type 'Test3Context'.
│ tests/generated/WhiteboxParser.ts(850,15): error TS2339: Property 'getToken' does not exist on type 'Test3Context'.
│ tests/generated/WhiteboxParser.ts(861,11): error TS2339: Property 'parser' does not exist on type 'Rule13Context'.
│ tests/generated/WhiteboxParser.ts(864,19): error TS2339: Property 'getTokens' does not exist on type 'Rule13Context'.
│ tests/generated/WhiteboxParser.ts(867,15): error TS2339: Property 'getToken' does not exist on type 'Rule13Context'.
│ tests/generated/WhiteboxParser.ts(870,19): error TS2339: Property 'getTokens' does not exist on type 'Rule13Context'.
│ tests/generated/WhiteboxParser.ts(873,15): error TS2339: Property 'getToken' does not exist on type 'Rule13Context'.
│  ELIFECYCLE  Command failed with exit code 2.
└─ Failed in 4.1s at /Users/ncordon/neo4j/cypher-language-support/vendor/antlr4-c3
/Users/ncordon/neo4j/cypher-language-support/vendor/antlr4-c3:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  [email protected] build: `pnpm generate && pnpm build-esm && pnpm build-commonjs`

Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

@OskarDamkjaer just realized pnpm publish does not seem to be distributing things correctly so we need to fix that before merging

@OskarDamkjaer OskarDamkjaer requested a review from ncordon April 15, 2025 21:42
@OskarDamkjaer OskarDamkjaer mentioned this pull request Apr 16, 2025
Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

Umm, there's still a few rough edges I think:

All of the packages seem to have the wrong version for the canary releases: 0.0.0?!

query-tools

I'd suggest reverting to the release-canary.js script if we cannot make it work with changeset.

Apart from that I can see the react-codemirror distributed package has fewer files? Are you sure that works? I'm trying to see whether that would work in the upx repo but I find it strange:

Before:
react-codemirror-before

Now:
react-codemirror-now

@OskarDamkjaer
Copy link
Collaborator Author

OskarDamkjaer commented Apr 16, 2025

Apart from that I can see the react-codemirror distributed package has fewer files? Are you sure that works? I'm trying to see whether that would work in the upx repo but I find it strange:

How did you check this? When I looked at what was published in verdaccio, those files were present. I only tried out if it'd install / import properly of the language-support package though. Tried running the react editor in a new toy project and it's struggling a little bit with the worker but the rest works. I'll look into it

Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

I imported this from upx and it was working and it looked differently the second time I downloaded it, so I'm happy to leave it as is 🚀

@OskarDamkjaer OskarDamkjaer merged commit eeb68e5 into main Apr 16, 2025
3 checks passed
@OskarDamkjaer OskarDamkjaer deleted the pnpm-move branch April 16, 2025 13:19
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.

2 participants