Skip to content
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

Exploration of PNPM #1249

Merged
merged 4 commits into from May 23, 2024
Merged

Exploration of PNPM #1249

merged 4 commits into from May 23, 2024

Conversation

area
Copy link
Member

@area area commented May 2, 2024

For whatever reason - possibly with the move to Node 20 - we're having real issues with a single lockfile being installable on Linux and OSX. This comes down to fsevents being only needed on OSX (and therefore optional elsewhere) and npm having an issue with that - see npm/cli#4828 for the ongoing issue related to this.

I've explored what it would look like if we moved to pnpm. As well as the lockfile issue being resolved (I think), a couple of nice side benefits:

  • Faster install times, which has been an issue with the CDapp dev environment
  • A 'proper' way to patch @nomiclabs/truffle-contract rather than a postinstall call to sed

npm i -g pnpm@8 to install, and then just use pnpm instead of npm to call everything is all that should be necessary.

NB based on top of maint/docker-file-builds.

@area
Copy link
Member Author

area commented May 2, 2024

As an addendum, after speaking to Chris, the resident Node guru:

  • He uses pnpm for ColonyJS, and is thinking of moving the dapp to it, so supports this 'wholeheartedly'.
  • He uses v8 there (as I do here), and will only look to move to v9 once it is more mature

@kronosapiens
Copy link
Member

kronosapiens commented May 3, 2024

I've been dealing with this issue for a while (~3 months) when we were on node@14. I went down a few rabbit holes, but the only fix which consistently worked in CI was manually editing package-lock.json to add "linux" to the os field:

      "os": [
        "darwin",
        "linux" // Manually add this
      ],

This behavior could conceivably be incorporated into a post-commit hook using sed.

@area area force-pushed the maint/docker-file-builds branch from 8400fd7 to 285be6d Compare May 3, 2024 08:48
@area area marked this pull request as ready for review May 3, 2024 14:23
Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

package.json Outdated Show resolved Hide resolved
@area area force-pushed the maint/pnpm branch 2 times, most recently from 1f1ae0f to fae6dfa Compare May 6, 2024 09:47
@area area force-pushed the maint/docker-file-builds branch from fcab0ac to 564289d Compare May 6, 2024 14:52
@area area force-pushed the maint/pnpm branch 3 times, most recently from ce55567 to 10356ab Compare May 8, 2024 10:41
@area area force-pushed the maint/docker-file-builds branch from 04ed490 to 0e28454 Compare May 8, 2024 15:25
Base automatically changed from maint/docker-file-builds to develop May 8, 2024 16:33
@area
Copy link
Member Author

area commented May 8, 2024

Don't want this merged yet (requires JoinColony/colonyCDapp#2316), but interested in a review.

Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

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

Looks reasonable, the patching functionality is quite interesting

kronosapiens
kronosapiens previously approved these changes May 13, 2024
@area
Copy link
Member Author

area commented May 22, 2024

The versioning check script is broken, but not as a result of this PR so don't intend for it to hold it up.

@area area merged commit 5c0a602 into develop May 23, 2024
2 checks passed
@area area deleted the maint/pnpm branch May 23, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants