Skip to content

feat: add pnpm and changesets to manage repository #97

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nmerget
Copy link
Contributor

@nmerget nmerget commented Jun 1, 2025

closes #95

  • This PR adds pnpm to manage the creation of all .ts files inside scripts folder with pnpm build
  • Removed scripts/out should be build in pipeline or locally to make sure everything is up to date
  • Add changesets for release management:
    • Easy Changelog for Open Source projects
    • Detailed descriptions with Markdown
    • Changes the jsb_version.h automatically
  • Add CONTRIBUTING.md

@Benjamin-Dobell
Copy link
Collaborator

What's the motivation behind uploading and downloading the JS runtime instead of just building it?

I can understand the motivation for v8, since it lives outside of the repository. However, I'm not sure I understand re: the runtime.

Is there potential for race conditions here if multiple builds are kicked off and replace the runtime artifacts?

@nmerget
Copy link
Contributor Author

nmerget commented Jun 6, 2025

It's possible to move the builds for the jsb runtime to a custom action and run it in every OS build. But it reuqires to setup node/pnpm every time. Furthermore, we need to move down to modules/GodotJS/ to run pnpm build.

By moving it to a separate workflow_call we reduce the build time but we have the upload/download stuff.
Another advantage is that you can download the generated files for debugging (only if there are issues in the pipeline).

We can create a custom action instead I don't have a preference

@chucksellick
Copy link

chucksellick commented Jun 9, 2025

Just a comment from personal experience ... pnpm is not significantly better than npm, especially not for a project with so few npm dependencies. And the latest versions of npm have workspace support out of the box. pnpm just means someone might have to install an additional tool to be able to build locally where npm will work fine. (I have also seen significant issues with pnpm and bin paths not working and so on, weird version conflict resolutions that didn't happen with npm, finding myself unable to upgrade pnpm, and probably other stuff)

Unless there is an obvious use case for pnpm that npm can't satisfy, I'd just suggest sticking with npm which any Typescript developer is guaranteed to already have installed.

@nmerget
Copy link
Contributor Author

nmerget commented Jun 10, 2025

Just a comment from personal experience ... pnpm is not significantly better than npm, especially not for a project with so few npm dependencies. And the latest versions of npm have workspace support out of the box. pnpm just means someone might have to install an additional tool to be able to build locally where npm will work fine. (I have also seen significant issues with pnpm and bin paths not working and so on, weird version conflict resolutions that didn't happen with npm, finding myself unable to upgrade pnpm, and probably other stuff)

Unless there is an obvious use case for pnpm that npm can't satisfy, I'd just suggest sticking with npm which any Typescript developer is guaranteed to already have installed.

Personally I like pnpm, but it's totally fine to use npm here. What's your opinion @Benjamin-Dobell?

@nmerget nmerget marked this pull request as draft June 10, 2025 07:10
@Benjamin-Dobell
Copy link
Collaborator

Benjamin-Dobell commented Jun 10, 2025

Mmm. I tend to avoid these conversations 😅 It's sort of like asking "why use GodotJS when there's already GDScript?"

Personally, I use pnpm for my own projects, so it's what I would have picked. But I'm not tied to it.

That said, I don't think it's accurate to say developers already have npm and don't have pnpm. Package managers themselves have versions, each project will (or at least should) be using a specific version. So a package manager is very likely going to have to be downloaded regardless. Since corepack is bundled in modern Node, it's really no hassle either way.

@chucksellick
Copy link

That said, I don't think it's accurate to say developers already have npm and don't have pnpm.

Surely npm is still installed as part of node? You'd have to go to extra lengths to not have it.

@chucksellick
Copy link

That said, I don't think it's accurate to say developers already have npm and don't have pnpm. Package managers themselves have versions, each project will (or at least should) be using a specific version. So a package manager is very likely going to have to be downloaded regardless. Since corepack is bundled in modern Node, it's really no hassle either way.

Well it turns out I didn't even know what corepack did :) But it also turns out it's being removed from the distribution in node 25+. https://www.reddit.com/r/node/comments/1jk3vhn/what_are_the_ramifications_of_corepack_being/

Copy link
Collaborator

@Benjamin-Dobell Benjamin-Dobell left a comment

Choose a reason for hiding this comment

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

I'm going to have to get used to the new workflow. But I think we should merge this and tweak if we run into problem.

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.

Improve release management
3 participants