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

[Proposal] Inline loaders #154

Closed
mcandeia opened this issue Apr 9, 2023 · 13 comments
Closed

[Proposal] Inline loaders #154

mcandeia opened this issue Apr 9, 2023 · 13 comments
Assignees
Labels

Comments

@mcandeia
Copy link
Contributor

mcandeia commented Apr 9, 2023

Inline loaders

Author: Marcos Candeia (@mcandeia)
State: Ready for Implementation

Overview

Loaders are a powerful way to fetch data from APIs. Currently, loaders are just ordinary functions that have access to the request and receive configuration data as well as sections and live (either) inside the functions folder (or loaders soon). They were created for a single purpose: to create a clear separation between where the data comes from and what the component data shape is. We can take the current implementation of Fashion's ProductShelf as an example:

export interface Props {
  title: string;
  products: Product[] | null;
  itemsPerPage?: number;
}

function ProductShelf({
  title,
  products,
}: Props) {
// [redacted code]
}

Note that the ProductShelf itself does not know where the Product[] comes from. The interesting part here is that the ProductShelf's Product[] would come from different APIs and even different e-commerce platforms, such as VTEX or Shopify.

Because of the nature of these components, where they define the shape of the data creating this clever separation, we're going to call them Universal Components. Universal Components are components that do not depend on any specific API; instead, they depend on the shape of the data. In fact, there are at least four different implementations for the Product[] that you can find in the std package. They are: VNDAProductList, VTEXProductList, VTEXLegacyProductList, and ShopifyProductList. This is only possible because we have a common ground type from schema.org named Product, which is declared once and imported from the implementer modules.

Notice that this is only possible because we have inverted the dependency order. Instead of the component depending on the data loader, the data loader depends on the component data shape, and so it can fetch data and convert it to the common ground type. In the case of the ProductShelf, the common ground type is the schema.org Product. However, it could be any ordinary TypeScript type that sections define or depend on. More importantly, you can have different packages implementing loaders that are unknown from the section's point of view, but they are suitable to be used as a Product[] loader (a.k.a. "interchangeable"), meaning that Universal Components are extensible by default.

Below you can see an explanation of how it looks in terms of imports:
image

This proposal suggests adding a new type of loader: the inline loader. They are supposed to be used in scenarios where interchangeability is not paramount (e.g., 80% of landing pages). They are not meant to replace the current loader implementation. Instead, they should be used as a complement. In fact, there are scenarios where they can be used together.

Background

Start simple and add new abstractions gradually

Even considering that universal components are powerful abstractions we need to take in consideration that we should have a way to start simpler and gradually adding abstractions as they are necessary. Currently, we are enforcing developers to create loaders separated from sections, which, generally makes the developer loses the focus on the code to go to the Editor admin to see it in action, see the current data-fetching documentation and notice that there are parts which the admin is required even for the simpler use case of fetching data.

Code localization

By allowing developers to fetching data inside the same place where UI components are created is great for DevX, often called "Colocalization" (sorry, in portuguese it sounds slighly better "Colocalização"), since they don't need to open multiples files/directory to understand how its code connects all together, requiring less cognitive effort to understand the "big picture".

People are trying to use loaders as a function (trying to invoke them)

This is pure empiricism but it's not so unusual to see people asking how to use the loader X or Y inside a section, trying to call the function itself.

Detailed Design

Inline loaders are just ordinary loaders but written within the same section (or island) file, they are invoked first, and when chosen, the section props are not used, instead the loader props will be used to be fulfilled in the Editor Admin, which means that the developers should declare type which contains properties that will be used by the loader itself and also "passing properties" which are those that will be used only for the target section/component. To show how it will look like, let's rewrite the entire data-fetching documentation by removing the loader and using the inline loader.

Important note: Loaders and inline loaders has the same signature on purpose to make possible refact the code as easy as just copying and pasting the loader code inside the loaders folder.

The final code implementation for the DogFacts tutorial is the following;

a dogApiFacts.ts file inside /loaders folder with the following content:

import type { LoaderContext } from "$live/types.ts";

// Return type of this loader
export interface DogFact {
  fact: string;
}

// Props type that will be configured in deco.cx's Admin
export interface Props {
  numberOfFacts?: number;
}

export default async function dogApiFacts(
  _req: Request,
  { state: { $live: { numberOfFacts } } }: LoaderContext<Props>,
): Promise<DogFact[]> {
  const { facts } = (await fetch(
    `https://dogapi.dog/api/facts?number=${numberOfFacts ?? 1}`,
  ).then((r) => r.json())) as { facts: string[] };
  return facts.map((fact) => ({ fact }));
}

A section named DogFacts.tsx inside /sections folder with the following content:

import { DogFact } from "../functions/dogApiFacts.ts";

export interface Props {
  title: string;
  dogFacts: DogFact[];
}

export default function DogFacts({ title, dogFacts }: Props) {
  return (
    <div class="p-4">
      <h1 class="font-bold">{title}</h1>
      <ul>
        {dogFacts.map(({ fact }) => <li>{fact}</li>)}
      </ul>
    </div>
  );
}

To see it in action you need to go to the admin and configure the section by selecting the created loader and also fulfill the section props, which is not too complex, but requires at least two files just because we decided to fetch data.

The dogFacts scenario is a perfect fit where you can just fetch from dogFacts APIs without worrying about being interchangeable and it is not so common to have a replacement for it in the short term (specially for a tutorial).

So let's rewrite them;

  1. Remove the dogApiFacts.ts from /loaders folder
  2. Rewrite the DogFacts.tsx section to
import type { LoaderContext } from "$live/types.ts";
import type { SectionProps } from "$live/mod.ts";

export default function DogFacts(
  { title, dogFacts }: SectionProps<typeof loader>,
) {
  return (
    <div class="p-4">
      <h1 class="font-bold">{title}</h1>
      <ul>
        {dogFacts.map((fact) => <li>{fact}</li>)}
      </ul>
    </div>
  );
}

// Props type that will be configured in deco.cx's Admin
export interface LoaderProps {
  title: string;
  numberOfFacts?: number;
}

export async function loader(
  _req: Request,
  { state: { $live: { numberOfFacts, title } } }: LoaderContext<LoaderProps>,
) {
  const { facts: dogFacts } = (await fetch(
    `https://dogapi.dog/api/facts?number=${numberOfFacts ?? 1}`,
  ).then((r) => r.json())) as { facts: string[] };
  return { dogFacts, title };
}

Notice a few things:

  1. The section props will not be used in the Editor Admin anymore, instead, the loader props will be used.
  2. As previously mentioned, the LoaderProps should contain any information that is needed to Fetch data and passing properties to the target section, so it should receive title and pass to the target section even when title is not being used directly.

To solve scenarios like this I'm also proposing an advanced usage of loader called PropsLoader which is basically a way to define a loader for a single property (or selected/multiple) and let the other properties being passed automatically by the framework. Let’s rewrite the previous example again;

import { PropsLoader } from "$live/mod.ts";
import type { LoaderContext } from "$live/types.ts";

// Props type that will be configured in deco.cx's Admin
export interface LoadProps {
  title: string;
  numberOfFacts?: number;
}

async function dogFacts(
  _req: Request,
  { state: { $live: { numberOfFacts } } }: LoaderContext<LoadProps>,
): Promise<string[]> {
  const { facts } = (await fetch(
    `https://dogapi.dog/api/facts?number=${numberOfFacts ?? 1}`,
  ).then((r) => r.json())) as { facts: string[] };
  return facts;
}

export interface Props {
  title: string;
  dogFacts: string[];
}

export default function DogFacts({ title, dogFacts }: Props) {
  return (
    <div class="p-4">
      <h1 class="font-bold">{title}</h1>
      <ul>
        {dogFacts.map((fact) => <li>{fact}</li>)}
      </ul>
    </div>
  );
}

export const loader: PropsLoader<
  Props,
  LoadProps
> = {
  dogFacts,
};

The code is very similar, but you can notice that

  1. The loader is simpler and can be extracted more easily because the signature (input/output) is exactly what is expected from the current tutorial implementation
  2. The other props can be just omitted and will be passed by the framework

I believe that this second option would also be used when multiples fetches must be invoked and the framework handles it in parallel instead of delegating this to the dev.

Expectations and alternatives

Async components

Async components were an alternative simpler in terms of usability because they will let developers to make fetch calls inside component once (at startup) and use the data in a closure function, see the example below;

// Props type that will be configured in deco.cx's Admin
export interface Props {
  title: string;
  numberOfFacts?: number;
}

export default async function DogFacts(
  { title, numberOfFacts }: Props,
) {
  const { facts } = (await fetch(
    `https://dogapi.dog/api/facts?number=${numberOfFacts ?? 1}`,
  ).then((r) => r.json())) as { facts: string[] };
  return () => (
    <div class="p-4">
      <h1 class="font-bold">{title}</h1>
      <ul>
        {facts.map((fact) => <li>{fact}</li>)}
      </ul>
    </div>
  );
}

What I didn't like in this approach is the fact that is needed to wrap the return inside a parameterless function () => which will probably lead in many hard-debugging bugs, because if the dev forgot to add the closure function it will work without any fresh hook, causing a lot of confusion, and it's not trivial and elegant to return a closure function, so I decided (together with the team) to not consider this possibility.

Completion checklist

[ ] Change section block to allow loader function
[ ] Update documentation

@mcandeia mcandeia self-assigned this Apr 9, 2023
@mcandeia
Copy link
Contributor Author

mcandeia commented Apr 9, 2023

WIP PR: #153

@lucis
Copy link
Contributor

lucis commented Apr 9, 2023

I really like the simplicity of it!

Inline loaders are awesome

@guifromrio
Copy link
Contributor

guifromrio commented Apr 9, 2023

This an amazing step.

I have one nit: I think we need to make it simpler to understand that I can "substitute" one of the props of my section for an async inline loader (instead of receiving through props or receiving through an external loader).

What I mean is: it's not trivially clear that LoaderProps will pass through and some of it will "fall down" to Props.

When I think about it, it seems to me that the easy way to explain it is something like:

  • You can have a static component - no props
  • You can have a component that receives static props - configure them on admin
  • You can have a component that loads part of the props with I/O - is this the level we're discussing?
  • And finally, you can have a component that loads part of the props with I/O and you can configure how that loads independently in the admin and reference it. (Is this where we started with loaders?)

Am I making any sense? :)

import { PropsLoader } from "$live/mod.ts";
import type { LoaderContext } from "$live/types.ts";

export interface Props {
  title: string;
  dogFacts: string[];
}

export default function DogFacts({ title, dogFacts }: Props) {
  return (
    <div class="p-4">
      <h1 class="font-bold">{title}</h1>
      <ul>
        {dogFacts.map((fact) => <li>{fact}</li>)}
      </ul>
    </div>
  );
}

export const loader: PropsLoader<Props> = {
  dogFacts: async function(_req: Request, { state: { $live: { numberOfFacts } } }: LoaderContext<LoadProps>,): Promise<string[]> {
    const { facts } = (await fetch(
      `https://dogapi.dog/api/facts?number=${numberOfFacts ?? 1}`,
    ).then((r) => r.json())) as { facts: string[] };
    return facts;
  },
};

Perhaps my question is: if you're an inline loader, why would you need a different set of properties? you're inline. You are just a part of a section. When you start requiring properties that make sense for you but not for your section, bingo: you have arrived at the need to extract a loader.

How does that sound?

@mcandeia
Copy link
Contributor Author

mcandeia commented Apr 9, 2023

Thanks for the feedback @guifromrio,

What I mean is: it's not trivially clear that LoaderProps will pass through and some of it will "fall down" to Props.

I agree but just to make it clear: It should be typesafe. The props loader will tell the developer which properties are "required" (e.g)

If title is optional in LoaderProps but required in SectionProps so the propsLoader MUST return a title regardless (in this case might be use a default value in case of not provided)

LoaderProps SectionProps Optional?
Optional Optional Yes
Optional Required No
Required Optional Yes
Required Required Yes
Missing Prop Optional Yes
Missing Prop Required No

Perhaps my question is: if you're an inline loader, why would you need a different set of properties? you're inline. You are just a part of a section. When you start requiring properties that make sense for you but not for your section, bingo: you have arrived at the need to extract a loader.

How does that sound?

If I understood your points correctly, you're saying that the loader should receive the same set of properties as the section, right? If so, in your example where the numberOfFacts comes from? (LoaderProps)

Isn't this a new property that should makes the dev extract the loader ?

Correct me If I'm wrong, and thanks again for the detailed feedback!

@guifromrio
Copy link
Contributor

Yes, I think that if you are uncomfortable having numberOfFacts as part of your Section Props API, then you are saying that this is not the correct abstraction layer for that information — therefore, go and extract a Type and implement an external TypeLoader. That can receive it's own props, have it's own block lifecycle, be referenced, etc. WDYT?

@mcandeia
Copy link
Contributor Author

mcandeia commented Apr 9, 2023

Understood, need to ask others opinion's, @lucis ?

Personally, I'm afraid the developers will add "unused" properties on purpose just to be able to use the inline loader, you know? An alternative would be generating the following:

type AdminForm = SectionProps | LoaderProps.

If SectionProps is fulfilled so it will be used instead (or the loader can do products: props.products ?? await fetchProducts()), but not sure if this will cause confusion either

@mcandeia
Copy link
Contributor Author

mcandeia commented Apr 9, 2023

So the way I see is the following:

  1. You can have a static component - no props
  2. You can have a component that receives props - configure them on admin (either coming from loaders or from inline data, I prefer to not separate async - not async, it does not matter from the section's point of view e.g we can have "fromUrl/fromState" loader which is not async)
  3. You can have a component that depends on external data and then write an inline loader - thus be aware that you cannot swap out the fetching part

In other words: I think the dev should not worry about fetching data when creating a Universal Component - a component that just receives props and makes no IO (or it can but configurable). There are devs that will build components to be used across repositories (reusable blocks) there are devs that will create components to be used in multiple places but within the same repo - this is the perfect usecase of inline data fetching

Notice that you can still depends on product[] and leveraging inline loader (which means that a component can be Universal but "impure" ?)

@tlgimenes
Copy link
Contributor

tlgimenes commented Apr 10, 2023

React Server Component is going towards an approach like the async componente proposition. Maybe, we can be smart in the engine and make an async component like signature work without needing the extra closure. This would be nice since we would be future proof. I'd like to write something on the lines:

async function Section() {
  const response = await fetch()
  
  return <div>{response.data}</div>;
}

@igorbrasileiro
Copy link
Contributor

async function Section() {
  const response = await fetch()
  
  return <div>{response.data}</div>;
}

IMO, this approach seems reasonable because the framework only has one way to express props to the admin form. The Fetch inside component is just a "fetch" and not a "inline loader".

@mcandeia
Copy link
Contributor Author

mcandeia commented Apr 10, 2023

React Server Component is going towards an approach like the async componente proposition. Maybe, we can be smart in the engine and make an async component like signature work without needing the extra closure. This would be nice since we would be future proof. I'd like to write something on the lines:

async function Section() {
  const response = await fetch()
  
  return <div>{response.data}</div>;
}

I agree. But I tried it using a few different ways without success. Looks like that I can't invoke the function with params and use the response directly due to the lack of fresh hooks that should be attached before the rendering. Invoking the function will make fresh have no chance to jump and do its magic,

So @igorbrasileiro and @tlgimenes, I'll be very happy if you guys can go ahead help me on achieve this on the next steps.

My proposal is: Since this is a technical limitation for now, I'd rather prefer to be pragmatic and select between the real possibilities.

Notice that changing from the current proposal to the one that you mentioned is not a breaking change, thus can be a step moving forward as soon as we realize how to make this work.


I'd like to share some thoughts that isn't obvious when moving from async components proposal that does not make it worse, but makes both proposal very similar in terms of advantages and disavantages:

  1. When separating data-fetching from components we make refactoring code easier. Sections are just sections and loaders are just loaders, they're not coupled together, meaning that the component and the loader can be reused separately. (imported from another section). Notice that an async section will also be a "deco-only" concept. Assuming that the section won't be reusable across code
  2. Islands and sections can be isomorphic, we can also provide a inline loader for islands that happen on server-side in the same way we do for sections. However, making islands async is a no-go, not only speaking in terms of limitations but also in terms of composability, islands is a fresh concept and we can cause confusion when trying to make them async.

@guitavano
Copy link
Contributor

I don't know how to contribute technically, but if help you in a decision. I like idea to fetch data inside a section without loaders.

Loaders are powerful, but I think we need to give the opportunity to do amazing things without them. At Hackathon, that was a rock in the learning courve of new devs.

I like the first code structure showed in this proposal, reminds me the "getServerSideProps" of next.js and looks like easy to use.

@mcandeia
Copy link
Contributor Author

Great @guitavano, this proposal is already merged and ready to be used! Thanks for your contribution!

@mcandeia
Copy link
Contributor Author

Available since 1.0.0-rc.53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants