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.

Sign up for GitHub

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

Jump to bottom

Bugfix #7070 Middleware not working with endpoints #7079

Closed
wants to merge 3 commits into from
Closed

Bugfix #7070 Middleware not working with endpoints #7079

wants to merge 3 commits into from

Conversation

alex-sherwin
Copy link
Contributor

Changes

  • when middleware is used with endpoints, defer execution of renderEndpoint until the middleware invokes next() (making it properly chainable, like it is for pages)
  • standalone type definitions for endpoint return types so that the types can be reused for a few purposes
  • abstract conversion of endpoint "simple" return type into a Response into a reusable function
  • modify endpoint middleware next() implementation, which invokes renderEndpoint, to convert any simple result into a Response and log a warning. This is necessary because middleware handlers are typed to only support receiving a Response instance from the result of next().
  • add unit test coverage
  • expand the @examples/middleware to show usage with endpoint scenarios

I looked into changing middleware handlers to all needing to handle Response | EndpointOutput, but that was a deep rabbit hole that would have potentially breaking changes for any existing middleware handler code out in the wild.

So, instead, to make middleware "just work" for all use cases, in a sane way people would expect (which is that middleware should ALWAYS run, so you can trust it for things like auth/session checks, etc), I added an automatic conversion from any endpoint "simple" result into a Response immediately after it's resolved via next() using the same process that App.callEndpoint used, abstracting that functionality into a common utility function.

Testing

Unit tests were added which cover:

  • middleware + endpoint with Response result
  • middleware + endpoint where middleware intercepts and returns a Response directly, never invoking next() (thus the API endpoint is never executed)
  • middleware + endpoint with "simple" result using text/plain;charset=utf-8 encoding
  • middleware + endpoint with "simple" result using application/json;charset=utf-8 encoding derived from path extension

Docs

/cc @withastro/maintainers-docs for feedback!

@alex-sherwin
#7070 - middleware run on endpoints properly
63aea89 
* when middleware is used with endpoints, defer execution of **renderEndpoint** until the middleware invokes next() (making it properly chainable, like it is for pages)
* stronger type definitions for endpoint return types so that the types can be reused for a few purposes
* abstract conversion of endpoint "simple" return type into a Response into a reusable function
* modify endpoint middleware next() implementation, which invokes **renderEndpoint**, to convert any simple result into a Response and log a warning
* add unit test coverage
* expand the @examples/middleware to show usage with endpoint scenarios
@alex-sherwin
switch to tabs to match old code
86df411
@changeset-bot
Copy link

changeset-bot bot commented May 12, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 72d61a4

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

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) labels May 12, 2023
@alex-sherwin alex-sherwin mentioned this pull request May 12, 2023
Closed
1 task
@alex-sherwin
Copy link
Contributor Author

alex-sherwin commented May 12, 2023 •
edited
Loading

I'm not sure why the checks are failing on:

@astrojs/deno:test: [commonjs--resolver] Cannot bundle Node.js built-in "fs" imported from "../../../../../astro/dist/core/util.js". Consider disabling ssr.noExternal or remove the built-in dependency.
@astrojs/deno:test:  error   Cannot bundle Node.js built-in "fs" imported from "../../../../../astro/dist/core/util.js". Consider disabling ssr.noExternal or remove the built-in dependency.

I didn't change this import on util.ts, and the tests run locally OK for me.

I re-pushed a commit to get these checks to re-run and they failed again for the same reason

Are the GitHub CI tests flaky, or did my PR really change something tangible causing this?

@alex-sherwin alex-sherwin changed the title Bugfix/7070 Bugfix #7070 Middleware not working with endpoints May 15, 2023
ematipico
ematipico reviewed May 16, 2023
View reviewed changes
packages/astro/src/core/endpoint/index.ts
} else {
warn(
logging,
'ssr',
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to use ssr as logging instead of middleware?

packages/astro/src/core/endpoint/index.ts
@@ -16,21 +16,24 @@ import { AstroError, AstroErrorData } from '../errors/index.js';
import { warn, type LogOptions } from '../logger/core.js';
import { callMiddleware } from '../middleware/callMiddleware.js';
import { isValueSerializable } from '../render/core.js';
import { simpleEndpointOutputToResponse } from '../util.js';
Copy link
Member

Choose a reason for hiding this comment

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

We can't import util in this file because this file relies on node: specific utilities. It's preferable to move simpleEndpointOutputToResponse somewhere else, where node is not used.

packages/astro/src/core/util.ts
@@ -221,3 +224,31 @@ export function resolvePath(specifier: string, importer: string) {
return specifier;
}
}

// EndpointOutput.body is typed as "Body" (not exported) which is only allowed to be a string
export function isEndpointOutput(o: any): o is EndpointOutput {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we can give a better name instead of o. Maybe "responseOutput" works better

@ematipico
Copy link
Member

I'm not sure why the checks are failing on:

@astrojs/deno:test: [commonjs--resolver] Cannot bundle Node.js built-in "fs" imported from "../../../../../astro/dist/core/util.js". Consider disabling ssr.noExternal or remove the built-in dependency.
@astrojs/deno:test:  error   Cannot bundle Node.js built-in "fs" imported from "../../../../../astro/dist/core/util.js". Consider disabling ssr.noExternal or remove the built-in dependency.

I didn't change this import on util.ts, and the tests run locally OK for me.

I re-pushed a commit to get these checks to re-run and they failed again for the same reason

Are the GitHub CI tests flaky, or did my PR really change something tangible causing this?

Here's why

@alex-sherwin
Copy link
Contributor Author

Ok I got it.

Based on your issue comment here: #7070 (comment)

Then, I'm happy to wait for an update from you/astro team.. as it stands right now I put some code in there which converts EndpointOutput to Response during the middleware processing, which I think is not desired (however, maybe it's ok in the interim until EndpointOutput is removed?)

For now I will update this PR so it works how I intended and let you decide what direction to go in, I'm happy to contribute more changes to align with that

@ematipico ematipico mentioned this pull request May 16, 2023
Merged
@alex-sherwin
Copy link
Contributor Author

Closing in favor of #7106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ematipico ematipico ematipico left review comments

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alex-sherwin @ematipico