-
Notifications
You must be signed in to change notification settings - Fork 42
patch node-module-loader dynamic require #431
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: df638ff 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 |
packages/cloudflare/src/cli/build/patches/plugins/node-module-loader.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/build/patches/plugins/node-module-loader.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/build/patches/plugins/node-module-loader.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/build/patches/plugins/node-module-loader.ts
Outdated
Show resolved
Hide resolved
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.
Thanks for the PR.
I added minor comments but what's this is really lacking is a test in e2e/pages-router. Can you please add one?
i'm not quite sure how i'd go about doing that. should i just create a test that fetches the api route and checks that the response was successful? |
That sounds great. |
commit: |
Maybe you can also move the 2 patches in the same file as they are very closely related. It will probably allow to factor some code too. |
if you don't have any more comments could this get merged? |
Have you tried the build of the adapter attached to this PR and can confirm it fixed your issue? Thanks for your work on the fix! |
If everything works as expected, please add a patch changeset (pnpm changeset and follow the prompt) |
Use pnpm fix to format the coffee but it looks like there are legit errors to fix first |
should be fixed now, seems like it was because of yaml not being as generous with whitespace, could you rerun the checks/workflows? i'll test it on my app with pkg.pr.new |
Use pnpm e2e to run the tests locally, formatting still looks off |
should be good to go now. the issue was that |
e2fe39d
to
df638ff
Compare
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.
Thanks!
should fix #380