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: support async route components #1388

Merged

Conversation

marvinhagemeister
Copy link
Collaborator

@marvinhagemeister marvinhagemeister commented Jul 1, 2023

This is a quick implementation for #1386 . It allows you to use an async component function where you can load data and return a vnode. If the function is async it will receive the request object and middleware context as arguments instead. We fall back to the current way if it's not async.

export default async function Home(req, ctx) {
  const data =  await getDataFromDB()
  return <App data={data} />;
}

@marvinhagemeister marvinhagemeister force-pushed the async_server_components branch 3 times, most recently from 04d6bde to 72145d4 Compare July 2, 2023 14:24
@marvinhagemeister marvinhagemeister marked this pull request as ready for review July 3, 2023 01:51
@marvinhagemeister
Copy link
Collaborator Author

Ah wait, marking as draft again because I want to add a couple of tests with ctx.render()

@marvinhagemeister marvinhagemeister marked this pull request as draft July 3, 2023 01:51
@lucacasonato
Copy link
Member

I think this breaks the twind plugin.

@flemmingmiguel
Copy link
Contributor

flemmingmiguel commented Jul 3, 2023

Can confirm that it breaks the twind plugin, also islands that are imported in async route components don't execute the revive, also can't import signals into the async route to pass to different island

otherwise is really promising I've tested it with Deno.KV executing calls inside the route and it works great

@marvinhagemeister
Copy link
Collaborator Author

Thanks for the feedback and giving this a go! I'll look into these issues.

@marvinhagemeister marvinhagemeister force-pushed the async_server_components branch from 60a4132 to 5d37d1c Compare July 5, 2023 07:56
@marvinhagemeister marvinhagemeister marked this pull request as ready for review July 5, 2023 08:12
@marvinhagemeister
Copy link
Collaborator Author

I think it's ready to be reviewed. I've tested it locally with porting our docs to that setup and islands + twind works there.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM! Just some nits. Am I right in understanding that this would almost eliminate the need for GET middleware for rendered pages? If so, wow.

src/server/types.ts Outdated Show resolved Hide resolved
tests/server_components_test.ts Outdated Show resolved Hide resolved
tests/server_components_test.ts Outdated Show resolved Hide resolved
tests/server_components_test.ts Outdated Show resolved Hide resolved
tests/server_components_test.ts Outdated Show resolved Hide resolved
src/server/context.ts Outdated Show resolved Hide resolved
Comment on lines 171 to 174
if (
typeof component === "function" &&
component.constructor.name === "AsyncFunction"
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (
typeof component === "function" &&
component.constructor.name === "AsyncFunction"
) {
if (component.constructor.name === "AsyncFunction") {

This should suffice, no?

src/server/context.ts Show resolved Hide resolved
src/server/context.ts Outdated Show resolved Hide resolved
src/server/context.ts Outdated Show resolved Hide resolved
@marvinhagemeister
Copy link
Collaborator Author

@iuioiua Thanks for the review! Yes, this PR would eliminate the need for GET route handlers for rendered pages. It's optional though one can still keep the current model as well.

@bjesuiter
Copy link
Contributor

Hi @marvinhagemeister, I did also try the PR and it seems like it still has errors with Tailwind (at 2023-07-05, 1 pm in Germany).

You can view the result here: https://bjesuiter-fresh-playground.deno.dev/pokelist-async

I tried the following:

Step 1

First I added your branch directly from github to a "Fresh" Playground Repo of mine like this: https://github.com/bjesuiter/deno_fresh_playground#switch-to-arbitrary-fresh-version-on-github-wip
Note: It's awesome that deno works like this <3

Step 2

Second I added a page with the new async page components feature, which uses the Pokemon API to list some Pokemons:
Source: https://github.com/bjesuiter/deno_fresh_playground/blob/main/routes/pokelist-async.tsx

import {HandlerContext} from '$fresh/server.ts';
import {PokeList, Pokemon} from '../components/PokeList.tsx';

/**
 *
 * @param req Request
 * @param ctx HandlerContext
 * @returns
 */
export default async function AsyncPagePoc(req: Request, ctx: HandlerContext) {
	console.log(`In AsyncPageTest`);
	console.log(`\n ---- \n\n Req Object: `, req);
	console.log(`\n ---- \n\n Context Object: `, ctx);

	// const res = await fetch(`https://pokeapi.co/api/v2/pokemon/ditto`);
	const res = await fetch(`https://pokeapi.co/api/v2/pokemon?limit=10&offset=0`);

	// const jsObj = await res.json();
	const pokeList = (await res.json()).results as Pokemon[];

	const pokeListWithImagesAsync = pokeList.map(async poke => {
		const res = await fetch(poke.url);
		const body = await res.json();

		// https://pokeapi.co/api/v2/pokemon/1/
		poke.imgUrl = body.sprites.other.home.front_default;
		return poke;
	});

	const pokeListWithImages = await Promise.all(pokeListWithImagesAsync);

	return (
		<div class="max-w-80 mx-auto">
			{/* Tailwind seems not to work here, for some reason */}
			{/* Also tried class={tw`text-5xl`} */}
			<h1 class="text-5xl text-red-500">Async Page Test</h1>
			<PokeList entries={pokeListWithImages}></PokeList>
			{/* <pre>{JSON.stringify(jsObj, null, '\t')}</pre> */}
		</div>
	);
}

Step 3

I added a normal server-side component, called PokeList to render the list:
Source: https://github.com/bjesuiter/deno_fresh_playground/blob/main/components/PokeList.tsx

export type Pokemon = {
	name: string;
	url: string;
	imgUrl?: string;
};

export function PokeList({entries}: {entries: Pokemon[]}) {
	return (
		<>
			{/* Tailwind works here though! */}
			<h2 class="text-4xl">PokéList</h2>
			<ul class="my-8 w-56">
				{entries.map(poke => (
					<li class="relative m-4 p-6 shadow-xl text-center border-2 border-gray-400 rounded">
						{poke.imgUrl && <img src={poke.imgUrl}></img>}
						<a class="absolute top-2 left-0 right-0 capitalize" href={poke.url}>
							{poke.name}
						</a>
					</li>
				))}
			</ul>
		</>
	);
}

The tailwind problem

If you take a look now to my deployed version of this at https://bjesuiter-fresh-playground.deno.dev/pokelist-async (or run it locally yourself), you'll see that the tailwind classes from the async page component are not applied, but the tailwind classes in the server component will be applied correctly.

Bildschirmfoto 2023-07-05 um 13 08 36

=> The first heading "Async Page Test" should've been "text-5xl text-red-500" (as you see in the page src code above)
=> If I use "text-4xl" though, it works, since the server component "PokeList" does also use this class, so it's available in the output.

@marvinhagemeister
Copy link
Collaborator Author

@bjesuiter this was super helpful! Thanks for sharing your test case. Missed that plugins can contribute to rendering too earlier. Got that resolved now and the styles show up 🎉

@bjesuiter
Copy link
Contributor

bjesuiter commented Jul 5, 2023

Can confirm, Tailwind in general works now in Async Page Components. (See screenshot)

However, not all colors work. 'text-red-500' and 'text-green-500' work, but 'text-lime-500' and 'text-teal-500' do not.
(But, this might be a different issue and might not have anything to do with the async page ^^)

Update

This indeed seems to be a different problems, since these colors also do not work inside the PokéList Component.

Bildschirmfoto 2023-07-05 um 20 15 59

sheet.reset(undefined);
const res = ctx.render();
const res = await ctx.renderAsync();
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work. Twind relies on thread local state (like context or hooks). Try rendering two pages in parallel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point, I'll take a look.

@marvinhagemeister
Copy link
Collaborator Author

marvinhagemeister commented Jul 5, 2023

@bjesuiter yeah that's a difference between tailwind and twind. They are very similar but not the same. I think newer twind versions aligned more closely with tailwind, but I haven't looked into that yet. I'm hoping we can support both twind and tailwind at some point in the future.

@marvinhagemeister marvinhagemeister force-pushed the async_server_components branch from 3269d77 to 4075999 Compare July 12, 2023 14:33
@marvinhagemeister marvinhagemeister merged commit 62a5613 into denoland:main Jul 12, 2023
@marvinhagemeister marvinhagemeister deleted the async_server_components branch July 12, 2023 19:09
@bjesuiter
Copy link
Contributor

Wohoo, my next question would've been when this is going to be merged!

Does fresh now have some kind of release cycle or is it more like rolling release?

@marvinhagemeister
Copy link
Collaborator Author

Planning to do release on a monthly basis. The next one is scheduled for tomorrow if all goes according to plan. Currently writing the blog post, but it's taking me a bit longer than I'd like to admit 😅

@bjesuiter
Copy link
Contributor

Very nice! I'm so excited that fresh is picking up some steam!

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.

5 participants