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

make sure patchFindDir works with Next.js 15 and add patching validation check #184

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

dario-piotrowicz
Copy link
Contributor

resolves #172


The patchFindDir function:

export function patchFindDir(code: string, config: Config): string {

Stopped working since a new name variable got introduced in the Next.js source code, causing the bundled code to rename the function's name parameter to name2 instead:
Screenshot 2024-12-17 at 19 33 16

I fixed this by updating the code replacement to also accept the variables with a numerical suffix. I also added a check to make sure that the patching is applied (if we already had the check in place that would have saved me lots of time here! 😭)

A few notes:

  • I wanted to update the code to use AST manipulation, but since this patch is in the to-investigate directory I guess that we assume that this is temporary, so maybe it's not worth investing time to update the code to use ASTs? (if it is I am more than happy to do so!)
  • again, having the patchedCode !== code check would have saved me lots of time here, we need to add it to all the patch functions to avoid wasting time again, I will do that in a followup PR
  • potentially we can hit this issue with other patching functions, I can update those as well to accept numerical suffixes in their variables/paramenters, but I think that adding the patchedCode !== code check would be good enough for now

Copy link

changeset-bot bot commented Dec 17, 2024

⚠️ No Changeset found

Latest commit: 9d8ccd5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

pkg-pr-new bot commented Dec 17, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@184

commit: 9d8ccd5

`function findDir(dir, name) {
if (dir.endsWith(".next/server")) {
if (name === "app") {
const patchedCode = code.replace(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment here with the pre-15.1 code and 15.1 code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you sure? it feels a bit overkill to me here 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

It might help if we later need to convert the patch to AST but your call :)

Copy link
Contributor

Choose a reason for hiding this comment

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

(and to be clear I meant fn signature, not the whole function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd leave it as is if it's ok by you 🙂

but if you do prefer a comment, could you please add a code suggestion here and I'd be happy to just go with whatever you'd fine useful here 🙂

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Thanks Dario 🙏

Does this also resolve the warnings (eval(require)...?)

Feel to to assign #28 to yourself and add it to the board :)

@dario-piotrowicz
Copy link
Contributor Author

Does this also resolve the warnings (eval(require)...?)

no 🥲

I can look into those if you want, but since they are only warnings I assumed that they are lower priority

Regarding warnings we also get this one:
Screenshot 2024-12-17 at 19 56 47

@vicb
Copy link
Contributor

vicb commented Dec 17, 2024

Does this also resolve the warnings (eval(require)...?)

no 🥲

I can look into those if you want, but since they are only warnings I assumed that they are lower priority

Regarding warnings we also get this one: Screenshot 2024-12-17 at 19 56 47

Maybe create an issue for eval(require)... so that we look into it at some point.

process.env.NODE_ENV is because Open Next set the value to "production" while we also force a define with the same value with ESBuild. No big deal but something we should fix. It is listed as a KI in the readme for now.

@vicb vicb merged commit ea2fbfa into experimental Dec 17, 2024
6 checks passed
@vicb vicb deleted the dario/experimental/fix-patch-find-dir branch December 17, 2024 20:17
vicb pushed a commit that referenced this pull request Dec 20, 2024
Also add patching validation check
vicb pushed a commit that referenced this pull request Dec 20, 2024
Also add patching validation check
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.

2 participants