-
Notifications
You must be signed in to change notification settings - Fork 357
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
fix: correctly create edge function directory when running in custom cwd #4860
Conversation
📊 Benchmark resultsComparing with d074933 Package size: 222 MB(no change)
Legend
|
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.
Great!
return new Response(text.toUpperCase(), resp) | ||
}, | ||
name: 'yell', | ||
handler: async () => new Response('Edge Function works'), |
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.
Not a blocker at all, but I'm curious about why you changed the test here. The previous test edge function was a bit more comprehensive, in that it was calling the origin and making a transformation, whereas the new one is just delivering a response.
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.
Isn't that what we are testing here? That the function was deployed correctly and runs correctly?
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.
I can revert it if you want, but I thought this is simpler and does exactly what we are trying to test here. And I guess/hope we have some tests somewhere that asserts that the edge internals work.
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.
Isn't that what we are testing here? That the function was deployed correctly and runs correctly?
Yes, we're still covering that, and I understand that the functionality we're no longer testing here should be tested somewhere else. Still unclear to me why the change was needed, but that's okay.
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.
I can revert it if you want, but I thought this is simpler and does exactly what we are trying to test here. And I guess/hope we have some tests somewhere that asserts that the edge internals work.
No need to revert.
Summary
Fixes opennextjs/opennextjs-netlify#1400
Fixes netlify/pod-compute#150
The edge function dist path was incorrect when a custom working directory
--cwd
was supplied through the cli options. We were looking at/repo/path/.netlify/...
instead of/repo/path/cwd/.netlify/...
Added a new test (which failed without the fix) and adjusted the other test to be more clear what failed. Just getting the uppercase/lowercase error, usually made me think this is some unimportant server/proxy conversation, but it actually means the edge function is not being loaded. Should be clearer now.