Skip to content

use pnpm as package manager#1557

Merged
thet merged 1 commit into
masterfrom
petschki-pnpm
Mar 18, 2026
Merged

use pnpm as package manager#1557
thet merged 1 commit into
masterfrom
petschki-pnpm

Conversation

@petschki
Copy link
Copy Markdown
Member

@petschki petschki commented Mar 16, 2026

  • I signed and returned the Plone Contributor Agreement, and received and accepted an invitation to join a team in the Plone GitHub organization.
  • I verified there aren't any other open pull requests for the same change.
  • I followed the guidelines in Contributing to Plone.
  • I successfully ran code quality checks on my changes locally.
  • I successfully ran tests on my changes locally.
  • If needed, I added new tests for my changes.
  • If needed, I added documentation for my changes.
  • I included a change log entry in my commits.

@petschki petschki requested a review from thet March 16, 2026 13:12
Copy link
Copy Markdown
Member

@thet thet left a comment

Choose a reason for hiding this comment

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

Looks good, but a detail about npx pnpm.

Comment thread Makefile
export

YARN ?= npx yarn
PNPM = pnpm
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we should use npx pnpm here.... Because that way it's sure it's installed, as everone has a Node environment available.

Copy link
Copy Markdown
Member

@davisagli davisagli Mar 16, 2026

Choose a reason for hiding this comment

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

A common way to handle this in the Node ecosystem is with corepack: https://pnpm.io/installation#using-corepack

npx pnpm would not guarantee that you are using a particular version of pnpm.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I switched barceloneta to pnpm a while ago and used corepack there. See https://github.com/plone/plonetheme.barceloneta/blob/master/HOWTO_DEVELOP.md ... this is also used in the plone developer docs https://6.docs.plone.org/install/create-project-cookieplone.html#node-js ... I'd recommend to use corepack here too and update the README accordingly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, didn't know corepack. Nice. Then no problem!

I have ./node_modules/.bin (and other paths) in my PATH, so I don't activate environments, normally.

Comment thread webpack.config.js Outdated
// }
// XXX: this doesn't work. currently, webpack configuration needs "shamefully-hoist=true"
// which creates a flat node_modules directory.
// see this comment for a possible solution: https://github.com/webpack/webpack/issues/5087#issuecomment-3193480986
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm. Okay. But no problem I guess...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also think its not a problem. Packages are just not that efficiently symlinked like pnpm would do normally.

@thet
Copy link
Copy Markdown
Member

thet commented Mar 17, 2026

@petschki

I did some updates on this PR:

  • Added some more Makefile targets to override defaults from @patternslib/dev where still yarn is used.
  • Changed the yarn occurrences with equivalents for pnpm. All was just a swap with pnpm except the yarn.lock, wich is now a pnpm-lock.yaml.

I tried the command npx pnpm start:webpack and it looks OK to me - I'm asked if I want to download pnpm by corepack.
Is that also what you would expect when using corepack‌ @petschki ?
If not, please update the README.md with the correct usage.
But it looks OK to me:

image

When we merge this, we also need to update the plone documentation docs. DRAFT PR here: plone/documentation#2069

@thet
Copy link
Copy Markdown
Member

thet commented Mar 17, 2026

hrm. a test is failing.

@petschki
Copy link
Copy Markdown
Member Author

petschki commented Mar 17, 2026

I tried the command npx pnpm start:webpack and it looks OK to me - I'm asked if I want to download pnpm by corepack. Is that also what you would expect when using corepack‌ @petschki ? If not, please update the README.md with the correct usage. But it looks OK to me:
image

When we merge this, we also need to update the plone documentation docs.

This looks OK to me, but I always follow the plone docs for prerequisites here: https://6.docs.plone.org/install/create-project-cookieplone.html#node-js they do

npm i -g corepack@latest && corepack enable

@thet
Copy link
Copy Markdown
Member

thet commented Mar 17, 2026

In the plone docs I've also read that in recent Node.JS environments corepack is already included.
So maybe this recommendation is also out of date?

I have updated a draft PR for the docs update here: plone/documentation#2069

Also, once we have updated @patternslib/dev and co we could remove the Makefile customizations which I added in my recent commit.

@petschki
Copy link
Copy Markdown
Member Author

The pat-structure test ist flaky because it lasts very long. I tried to fix this with NODE_OPTIONS='--max-old-space-size=8192' but obviously its still not reliable. locally its no problem.

re-running the GHA fixed it.

@thet
Copy link
Copy Markdown
Member

thet commented Mar 17, 2026

Regarding corepack - I'm confused: It was added to the Node distribution in v14 and v16 but removed again in v25?

Copy link
Copy Markdown
Member

@thet thet left a comment

Choose a reason for hiding this comment

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

LGTM.

@thet
Copy link
Copy Markdown
Member

thet commented Mar 17, 2026

Regarding the corepack confusion. Not a problem that corepack was removed from Node again. The corepack package manager manager is installed as recommended - via the npm package manager.

@davisagli
Copy link
Copy Markdown
Member

When you put it like that, it does sound a little silly. :)

@thet thet merged commit 1e0aaba into master Mar 18, 2026
2 checks passed
@thet thet deleted the petschki-pnpm branch March 18, 2026 15:25
@thet
Copy link
Copy Markdown
Member

thet commented Mar 18, 2026

Let's merge and see what happens.

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