Switch `app.render` signature #9199

lilnasy · 2023-11-27T15:51:47Z

Changes

app.render(request, undefined, locals) -> app.render(request, { locals })

Testing

Switched a couple of tests to use the new API, keeping two as-is that still test the current API.

Docs

Adapter API reference: signature change in a function docs#5510

changeset-bot · 2023-11-27T15:51:52Z

🦋 Changeset detected

Latest commit: a756ff1

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

natemoo-re

Code looks good! Just a few thoughts on how we roll this out and talk about it, but nothing blocking.

natemoo-re · 2023-11-27T21:02:55Z

packages/astro/src/core/app/node.ts

 		if (!(req instanceof Request)) {
 			req = createRequestFromNodeRequest(req);
 		}
-		return super.render(req, routeData, locals);
+		return super.render(req, { routeData, locals });


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.

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

natemoo-re · 2023-11-27T21:03:53Z

packages/astro/src/core/app/index.ts

@@ -145,7 +151,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>


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.

You can deprecate an overload?!

natemoo-re · 2023-11-27T21:04:56Z

packages/astro/src/core/app/node.ts

+			}
+		}
+		else {
+			routeData = routeDataOrOptions as RouteData;


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

natemoo-re · 2023-11-27T21:06:50Z

.changeset/sharp-starfishes-compete.md

+In the Integration API, the `app.render()` method of the `App` class has been simplified.
+


I would rephrase this or add a note that this change is really only relevant for adapter/integration maintainers, not a something that users of official Astro adapters need to update.

Updated in the commit titled "clarify changeset".

bluwy

I think we also need major changesets for @astrojs/node and @astrojs/vercel since they're using a new syntax that won't be compatible with previous Astro versions anymore.

bluwy · 2023-11-28T10:04:56Z

packages/astro/src/core/app/node.ts

 		if (!(req instanceof Request)) {
 			req = createRequestFromNodeRequest(req);
 		}
-		return super.render(req, routeData, locals);
+		return super.render(req, { routeData, locals });


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.

Wouldn't that make it log the warning everytime?

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.

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.

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.

ok yeah that makes sense

bluwy

Looks great!

matthewp · 2023-11-29T13:12:51Z

@lilnasy can you fix the conflicts so we can get this in?

bluwy · 2023-11-29T15:36:56Z

@lilnasy feel free to merge this if it's ready. or if you're waiting for a docs review?

lilnasy · 2023-11-29T15:38:32Z

Yeah, it's ready to merge. I'm under the impression that docs will review 4.0 stuff at a later point (not in the next branch).

remove warning: [deprecated] The adapter @astro-aws/adapter is using a deprecated signature of the 'app.render()' method. From Astro 4.0, locals and routeData are provided as properties on an optional object to this method. Using the old signature will cause an error in Astro 5.0. See withastro/astro#9199 for more information.

* feat: update Astro render call for v4 The app.render call deprecates the third argument in favor of an object with teh routeData as a property. See withastro/astro#9199 for more information. * chore: add changeset * feat: add v3 render support * fix: resolve import issue with const file remove unused type * feat: add a safe fall-through on version parsing * Sync --------- Co-authored-by: Frank <wangfanjie@gmail.com>

github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Nov 27, 2023

lilnasy force-pushed the next-app-render branch 3 times, most recently from 6972b25 to 585ce6f Compare November 27, 2023 16:15

github-actions bot added the pkg: integration Related to any renderer integration (scope) label Nov 27, 2023

lilnasy mentioned this pull request Nov 27, 2023

Adapter API reference: signature change in a function withastro/docs#5510

Merged

lilnasy force-pushed the next-app-render branch from 6f0b63b to 40b0699 Compare November 27, 2023 16:58

lilnasy marked this pull request as ready for review November 27, 2023 17:13

lilnasy mentioned this pull request Nov 27, 2023

Astro 4.0 breaking changes tracker withastro/docs#5390

Closed

natemoo-re approved these changes Nov 27, 2023

View reviewed changes

bluwy reviewed Nov 28, 2023

View reviewed changes

bluwy approved these changes Nov 28, 2023

View reviewed changes

lilnasy added 8 commits November 29, 2023 13:17

feat(app): app.render optional object

00f8542

tests

f5996ea

update vercel and node

a56bf8a

update changeset

ebf2822

deprecation notice and loggin

2f81429

clarify changeset

5799c8d

add node, vercel changeset

2bad1b4

deduplicate code

a756ff1

lilnasy force-pushed the next-app-render branch from 583425d to a756ff1 Compare November 29, 2023 13:18

lilnasy merged commit 49aa215 into next Nov 29, 2023
13 checks passed

lilnasy deleted the next-app-render branch November 29, 2023 15:38

This was referenced Nov 29, 2023

[ci] release (beta) #9228

Merged

[ci] release #9307

Merged

Skn0tt mentioned this pull request Dec 5, 2023

feat(netlify): Netlify Adapter v4 withastro/adapters#84

Merged

nerdoza mentioned this pull request Dec 18, 2023

feat: update render call to support all Astro versions sst/sst#3591

Merged

sarah11918 mentioned this pull request Jan 14, 2024

fix: updates to use the new app.render call signature withastro/adapters#130

Merged

github-actions bot mentioned this pull request Jan 16, 2024

[ci] release withastro/adapters#131

Merged

mseele mentioned this pull request Jan 18, 2024

fix: prepare for astro 5.0 lukeshay/astro-aws#120

Merged

bizz84 mentioned this pull request Apr 22, 2024

[WARN] [deprecated] The adapter astro-vercel-edge is using a deprecated signature of the 'app.render()' method. mayank99/astro-vercel-edge#1

Closed

webdevleo mentioned this pull request Apr 22, 2024

Fix deprecated "app.render()" signature mayank99/astro-vercel-edge#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch `app.render` signature #9199

Switch `app.render` signature #9199

lilnasy commented Nov 27, 2023 •

edited

Loading

changeset-bot bot commented Nov 27, 2023 •

edited

Loading

natemoo-re left a comment

natemoo-re Nov 27, 2023

lilnasy Nov 28, 2023

natemoo-re Nov 27, 2023

lilnasy Nov 28, 2023

natemoo-re Nov 27, 2023

lilnasy Nov 28, 2023

natemoo-re Nov 27, 2023

lilnasy Nov 28, 2023

bluwy left a comment

bluwy Nov 28, 2023

lilnasy Nov 28, 2023

bluwy Nov 28, 2023

lilnasy Nov 28, 2023

bluwy Nov 28, 2023

lilnasy Nov 28, 2023

bluwy left a comment

matthewp commented Nov 29, 2023

bluwy commented Nov 29, 2023

lilnasy commented Nov 29, 2023

		In the Integration API, the `app.render()` method of the `App` class has been simplified.

Switch app.render signature #9199

Switch app.render signature #9199

Conversation

lilnasy commented Nov 27, 2023 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Nov 27, 2023 • edited Loading

🦋 Changeset detected

natemoo-re left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluwy left a comment

Choose a reason for hiding this comment

matthewp commented Nov 29, 2023

bluwy commented Nov 29, 2023

lilnasy commented Nov 29, 2023

Switch `app.render` signature #9199

Switch `app.render` signature #9199

lilnasy commented Nov 27, 2023 •

edited

Loading

changeset-bot bot commented Nov 27, 2023 •

edited

Loading