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: Allow overriding the proxying for external rewrite #630

Merged
merged 12 commits into from
Nov 19, 2024

Conversation

conico974
Copy link
Contributor

This PR add an option to override the old proxyRequest fn that was used for external rewrites.
It also automatically make the proxying when used in the external middleware, no need to do it in a converter or the wrapper.

By default it is the same as before, but it could use fetch for the proxying

@vicb PTAL, i haven't tested the fetch part, i'd appreciate some feedback on this

Copy link

pkg-pr-new bot commented Nov 14, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@630

commit: 8ea7d39

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.

Super quick review because I'm busy again today.
Will do a deeper review on Monday

packages/open-next/src/adapters/middleware.ts Outdated Show resolved Hide resolved
packages/open-next/src/core/requestHandler.ts Outdated Show resolved Hide resolved
packages/open-next/src/types/open-next.ts Outdated Show resolved Hide resolved
Copy link

changeset-bot bot commented Nov 15, 2024

🦋 Changeset detected

Latest commit: 8ea7d39

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@opennextjs/aws Patch
app-pages-router Patch
app-router Patch

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

@@ -124,7 +125,7 @@ export async function resolveOriginResolver(
* @__PURE__
*/
export async function resolveWarmerInvoke(
warmer?: LazyLoadedOverride<Warmer> | "aws-lambda",
warmer?: LazyLoadedOverride<Warmer> | IncludedWarmer,
Copy link
Contributor

Choose a reason for hiding this comment

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

you update l 115 in a similar way string -> IncludedOriginResolver

Copy link
Contributor

@vicb vicb Nov 15, 2024

Choose a reason for hiding this comment

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

l 101 as well.
Maybe use the same as l86 everywhere

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.

I added a few comments, LGTM.

conico974 and others added 3 commits November 18, 2024 13:05
@@ -26,6 +32,9 @@ export interface IPluginSettings {
| LazyLoadedOverride<OriginResolver>
| IncludedOriginResolver;
warmer?: LazyLoadedOverride<Warmer> | IncludedWarmer;
proxyExternalRequest?:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

  • use OverrideOptions["..."] for warmer and proxyExternalRequest
  • DefaultOverrideOptions<any, any> can also be changed for OverrideOptions, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DefaultOverrideOptions<any, any> can also be changed for OverrideOptions, right?

Yes

OverrideOptions["..."] for warmer and proxyExternalRequest

We can't for the warmer, it is not defined in OverrideOptions

Copy link
Contributor

Choose a reason for hiding this comment

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

Right I mixed with wrapper 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we can't use OverrideOptions for the wrapper and the converter, it cause a few issue with typescript.
I'll revert to DefaultOverrideOptions<any,any>

@@ -84,6 +91,10 @@ export async function buildEdgeBundle({
: "sqs-lite",
}
: {}),
originResolver:
Copy link
Contributor

Choose a reason for hiding this comment

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

add proxyExternalRequest

@vicb
Copy link
Contributor

vicb commented Nov 19, 2024

@conico974 can we merge this?

@conico974 conico974 marked this pull request as ready for review November 19, 2024 13:19
@conico974
Copy link
Contributor Author

@vicb Yeah if everything is ok for the fetch/cloudflare part

@vicb
Copy link
Contributor

vicb commented Nov 19, 2024

@vicb Yeah if everything is ok for the fetch/cloudflare part

It looks okay, I'll test this more thoroughly soon and fix any thing that would need to be fixed.

Does that sound ok?

@vicb
Copy link
Contributor

vicb commented Nov 19, 2024

(Having the dummy types will help me progress)

@conico974 conico974 merged commit b999c4e into opennextjs:main Nov 19, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Nov 19, 2024
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