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

feat: migrate from yarn to pnpm #3128

Closed
wants to merge 12 commits into from
Closed

feat: migrate from yarn to pnpm #3128

wants to merge 12 commits into from

Conversation

AlessioGr
Copy link
Member

@AlessioGr AlessioGr commented Aug 5, 2023

Description

I had to remove the dynamic version ranges from the package.json and pin all the versions, as pnpm updated some of the versions ranges. This broke some tests. Some versions probably did a minor update which was breaking. Fun!

I'd be fine with leaving them pinned, but we can add dynamic version ranges back. I'd use ~ instead of ^, though.

TODO:
We need to determine if using pnpm's node-linker=isolated (which is their default, recommended one) breaks serverless deployments, as written here. Possible workarounds (if it in fact is an issue) could be found here and here.

Another possible workaround, if determined to be an issue, would be to use node-linker=hoisted. The benefit of this would be that we then wouldn't need a workaround for this typescript bug anymore. However, in this exact typescript issue comment, he writes the following (point 5.):

"But that might not be the case; imagine you publish express-app to the npm registry and someone installs it in a project using isolated mode - then express-serve-static-core is not present and it could introduce TypeScript errors on their side!"

So, basically, it could cause issue for people installing payload with pnpm's isolated mode. This is probably not an issue if we keep the workaround I've already included in this PR, though. However,, isolated mode should be helpful for tackling phantom dependencies when we switch to Workspaces.

If we decide to thus switch to node-linker=hoisted, we should keep the workaround and create a github action which builds the application using isolated mode as well, to verify it still works there,

  • I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • Chore (non-breaking change which does not add functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Change to the templates directory (does not affect core functionality)
  • Change to the examples directory (does not affect core functionality)
  • This change requires a documentation update

Checklist:

  • I have added tests that prove my fix is effective or that my feature works
  • Existing test suite passes locally with my changes
  • I have made corresponding changes to the documentation

@AlessioGr AlessioGr changed the base branch from master to 2.0 August 22, 2023 21:35
@AlessioGr AlessioGr changed the base branch from 2.0 to master August 22, 2023 21:35
@AlessioGr
Copy link
Member Author

Merged into 2.0 with e24ad67

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.

1 participant