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

Switch app.render signature #9199

Merged
merged 8 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat(app): app.render optional object
  • Loading branch information
lilnasy committed Nov 29, 2023
commit 00f854241f6ea170f48a521f5fd4ebf191bd5bab
11 changes: 11 additions & 0 deletions .changeset/sharp-starfishes-compete.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
'astro': major
---

The adapter API now offers a simpler signature for rendering. The `render()` method on App now accepts an `options` object.

```diff
- app.render(request, undefined, locals)
+ app.render(request, { locals })
```
The current signature is deprecated but will continue to function until next major version.
24 changes: 22 additions & 2 deletions packages/astro/src/core/app/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ const responseSentSymbol = Symbol.for('astro.responseSent');

const STATUS_CODES = new Set([404, 500]);

export interface RenderOptions {
routeData?: RouteData;
locals?: object;
}

export interface RenderErrorOptions {
routeData?: RouteData;
response?: Response;
Expand Down Expand Up @@ -141,7 +146,23 @@ export class App {
return routeData;
}

async render(request: Request, routeData?: RouteData, locals?: object): Promise<Response> {
async render(request: Request, options?: RenderOptions): Promise<Response>
async render(request: Request, routeData?: RouteData, locals?: object): Promise<Response>
Copy link
Member

Choose a reason for hiding this comment

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

Adding a /** @deprecated See https://github.com/withastro/astro/pull/9199 */ comment above the old signature would be good so that users get a hint in their editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can deprecate an overload?!

async render(request: Request, routeDataOrOptions?: RouteData | RenderOptions, locals?: object): Promise<Response> {
let routeData: RouteData | undefined;

if (routeDataOrOptions && ('routeData' in routeDataOrOptions || 'locals' in routeDataOrOptions)) {
if ('routeData' in routeDataOrOptions) {
routeData = routeDataOrOptions.routeData;
}
if ('locals' in routeDataOrOptions) {
locals = routeDataOrOptions.locals;
}
}
else {
routeData = routeDataOrOptions as RouteData;
}

// Handle requests with duplicate slashes gracefully by cloning with a cleaned-up request URL
if (request.url !== collapseDuplicateSlashes(request.url)) {
request = new Request(collapseDuplicateSlashes(request.url), request);
Expand All @@ -152,7 +173,6 @@ export class App {
if (!routeData) {
return this.#renderError(request, { status: 404 });
}

Reflect.set(request, clientLocalsSymbol, locals ?? {});
const pathname = this.#getPathnameFromRequest(request);
const defaultStatus = this.#getDefaultStatusCode(routeData, pathname);
Expand Down
21 changes: 19 additions & 2 deletions packages/astro/src/core/app/node.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { RouteData } from '../../@types/astro.js';
import type { SerializedSSRManifest, SSRManifest } from './types.js';
import type { RenderOptions } from './index.js';

import * as fs from 'node:fs';
import { IncomingMessage } from 'node:http';
Expand Down Expand Up @@ -116,11 +117,27 @@ export class NodeApp extends App {
}
return super.match(req);
}
render(req: NodeIncomingMessage | Request, routeData?: RouteData, locals?: object) {
render(request: NodeIncomingMessage | Request, options?: RenderOptions): Promise<Response>
render(request: NodeIncomingMessage | Request, routeData?: RouteData, locals?: object): Promise<Response>
render(req: NodeIncomingMessage | Request, routeDataOrOptions?: RouteData | RenderOptions, locals?: object) {
let routeData: RouteData | undefined;

if (routeDataOrOptions && ('routeData' in routeDataOrOptions || 'locals' in routeDataOrOptions)) {
if ('routeData' in routeDataOrOptions) {
routeData = routeDataOrOptions.routeData;
}
if ('locals' in routeDataOrOptions) {
locals = routeDataOrOptions.locals;
}
}
else {
routeData = routeDataOrOptions as RouteData;
Copy link
Member

Choose a reason for hiding this comment

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

Should we log a deprecation warning here? Maintainers probably won't catch this change unless we have visible log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

}

if (!(req instanceof Request)) {
req = createRequestFromNodeRequest(req);
}
return super.render(req, routeData, locals);
return super.render(req, { routeData, locals });
Copy link
Member

Choose a reason for hiding this comment

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

Do any adapters extend from App? They might need to be updated as well, but I guess it's not urgent since this is backwards-compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only NodeApp extends from App. Cloudflare uses App as-is.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can pass this as super.render(req, routeData, locals); like before so that the warning handling is all handled within App, and we don't have to duplicate the validation in NodeApp?

We can also make logRenderOptionsDeprecationWarning private with a leading # then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that make it log the warning everytime?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand. Wouldn't the duplicate validation here also log the warning everytime? As long as the consumer of NodeApp (in this case @astrojs/node) uses the new API form, it shouldn't log the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't the duplicate validation here also log the warning everytime?

It doesn't. It only logs a warning when the old API is used.

However, if NodeApp.render in turn uses the base method in the old form, the base method will log a warning everytime no matter what form the adapter author used.

Copy link
Member

Choose a reason for hiding this comment

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

I think my initial suggestion was incorrect. I meant super.render(req, routeDataOrOptions, locals);. Perhaps that's where the confusion is 😅

If we simply pass the arguments back to super.render, the newer API would also be passed through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok yeah that makes sense

}
}

Expand Down