-
Notifications
You must be signed in to change notification settings - Fork 70
fix: enable using workerd process v2 #903
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
Conversation
🦋 Changeset detectedLatest commit: a5cb01d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, good job on the fast fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work, but I don't think this is the right fix. We should not patch things in aws here, at least not like that.
We should use this plugin https://github.com/opennextjs/opennextjs-aws/blob/main/packages/open-next/src/plugins/replacement.ts and just add an #override
around it in aws
I agree it's not ideal and we should improve this.
So let's merge this PR as is for now and think about a better way to support "common" + variants when we work on the adapters API. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So let's merge this PR as is for now and think about a better way to support "common" + variants when we work on the adapters API.
WDYT?
That's fine by me, but I still prefer the #override
solution with the additionalCodePatches
What i really don't like about this one is that there is no way to know from the aws repo that we are modifying this function somewhere else. Very easy to forget about it and break a release this way
Agreed. Hopefully we don't add more than what the I have create this issue to improve the current PR but let's merge this now to avoid new projects fail build for now. |
Thanks all for the review and feedback. As the comments say, we should improve the current implementation but I'll merge this to avoid breaking new projects. |
fixes #899
node:process
has a new implementation in workerd ("v2" enabled by default on 2025-09-15).v2 supports
process.chdir()
so it now errors when a non existent directory is passed (the cloudflare adapter passes "").(The
unenv
implementation used before was a no-op).