RFC: checkout and checkout-app merger

[typeofweb]

Objective

Merge checkout and checkout-app into checkout-app Next.js application.

Goals:

• simplify the repo
• simplify workflow
• simplify deployment

Non-goals:

• change behaviour of these apps apart from what’s necessary

Motivation

Currently, in order to deploy Saleor Checkout, one needs to:

1. Fork saleor-checkout
2. Import & deploy checkout-app to Vercel
3. Configure it in Saleor
4. Update env variables & redeploy to Vercel
5. Import & deploy checkout to Vercel

By merging checkout and checkout-app into a single app, we’ll be able to drop the last step.

This is particularly important because, as our onboarding experience shows, the existence of two apps and their similar names are confusing to our users.

Things to keep in mind

Why the apps were divided

These two apps were divided in order to make it easier for our users to keep track of remote changes in their forks of saleor-checkout. The idea was for them to easily pull changes into checkout-app while being able to modify checkout without any concerns.

Unfortunately, this goal is not really met right now: users still face merge conflicts in checkout when they pull from the upstream repository to update checkout-app.

checkout is an SPA

checkout-app is a Next.js application while checkout is a Single Page Application based on CRA. This distinction was introduced in order to make it easier for our users to reuse checkout, host on static hostings or embed it inside existing applications simply by using a component.

Design Proposal

Merge checkout into checkout-app.

We’ll keep the structure of checkout-app as is but create a separate directory just for checkout:

.
├── apps
│   ├── checkout
│   │   └── (…)   ⬅ render `checkout-storefront` package for backward compatibility
│   └── saleor-app-checkout
│       ├── README.md
│       ├── backend
│       ├── frontend
│       ├── (…) existing checkout-app folders
│       └── pages
│           └── checkout-spa.tsx   ⬅ this page renders Checkout SPA
└── packages
    └── checkout-storefront
        ├── (…)   ⬅ other files moved from existing `checkout`
        └── index.tsx   ⬅ export a single component

We’ll add one new page to checkout-app/pages folder – it’ll only render the <Root /> components from packages/checkout-storefront.

Why?

This way, users have a way to use checkout SPA without Next.js if they need to:

• They can copy it to their own react apps
• They can install the repo via npm or git submodules and use the <Root /> component
• They can next export to get a static website (I was able to get this working by using some hacks in Next.js – see below)

It’s also way simpler to deploy to Vercel and develop: there’s just a single application to deploy.

Example next.config.js

const withTM = require("next-transpile-modules")();

const exportCheckoutSpa = Boolean(process.env.EXPORT_CHECKOUT_SPA);

/**
 * @type {import('next').NextConfig}
 */
const checkoutSpaConfig = {
  images: {
    loader: "custom",
  },
  exportPathMap: async () => {
    return {
      "/": { page: "/checkout-spa" },
    };
  },
};

/**
 * @type {import('next').NextConfig}
 */
const checkoutAppConfig = {
  i18n: {
    locales: ["en-US", "pl-PL"],
    defaultLocale: "en-US",
  },
  images: { domains: ["localhost"] },

  // eslint-disable-next-line require-await
  async redirects() {
    return [
      {
        source: "/",
        destination: "/channels",
        permanent: true,
      },
    ];
  },
  experimental: {
    esmExternals: false,
  },
};

/**
 * @type {import('next').NextConfig}
 */
module.exports = withTM({
  ...(exportCheckoutSpa ? checkoutSpaConfig : checkoutAppConfig),
  reactStrictMode: true,
  eslint: {
    dirs: ["pages", "backend", "frontend", "config"],
  },
});

[lkostrowski]

I definitely agree that single Next app is a way into easier deployment and maintenance. However, I'd consider following concerns:

1. App & SPA are meant to be used in different scopes. App is "a widget" inside Dashboard, while SPA is a client facing storefront. App should be protected and not accessible for users. Are we sure we will not face problems with mixed concerns?
2. We should consider SAAS model - if we continue with multi-tenant checkout, we will have single deployment of storefront, but also single deployment of App. @timuric ?
3. Updates and merge conficts - I assume you want to keep _checkout-spa empty, so git-pulls will update only App code? Is it maintainable? How do we provide our boilerplate? I mean scenario: I fetch boilerplate -> remove SPA, replace with my own -> I update fork -> my SPA is conflicted. Maybe we should keep a monorepo, so we export <Root> from sibling package and our concern is only config/import?

[typeofweb]

App should be protected and not accessible for users. Are we sure we will not face problems with mixed concerns?

App will be just as "protected" as it is right now. Nothing really changes in this matter.

you want to keep _checkout-spa empty

No, I want to move checkout-spa into this directory.

I fetch boilerplate -> remove SPA, replace with my own -> I update fork -> my SPA is conflicted.

This is precisely what's happening to our users right now so the monorepo approach doesn't really solve this particular problem. It's just hard to ignore a single folder during merge conflicts resolution as it will be if we merge checkout-spa into checkout – still just a single folder to ignore.

from sibling package

Yes, I'm considering this purely for code aesthetics but I don't see how it solves any of the above problems.

[lkostrowski]

Regarding conflicts, I was thinking about something like this

checkout-monorepo/
├─ node_modules/
├─ apps/
│  ├─ checkout // this we want to update
│  ├─ saleor-spa // this is example, can be removed
│  ├─ my-custom-spa // this isn't Saleor, not get updated
├─ package.json

And one app uses another or we have a meta-app for that

But I believe it can be achieved without monorepo, with folder ignoring, so thats fine, also my idea may be overcomplicated already

App will be just as "protected" as it is right now.

Right, I'm just thinking a little bit on the future, not sure if I can give an example now.

Anyway, I think it's a good move, I can't find any reasons now not to merge them, I can find some benefits already. 👍

[witoszekdev]

Ad 1. That's not a problem. We can have a separate page that is meant to be opened only in dashboard and detect if it was by iframe in Saleor dashboard or directly by user, here's an example from saleor-app-template.

Ad 3. +1 to that, I think we should still treat checkout as a separate app in monorepo, that can be imported by checkout-app

[witoszekdev]

If merge conflicts are a conern then maybe we can drop the monorepo and instead use multiple different repos and an additional repo that has those repos as submodules like we do in saleor-platform (it includes Saleor Dashboard, Saleor Core and React Storefront as submodules)

[typeofweb]

So it seems the consensus is to keep checkout (SPA) as a separate package in the monorepo.
This has plenty of benefits:

• we export a package with components which can be used by our users
  • we could even publish it to npm in the future
• we can easily be backward-compatible by just reusing that package inside the existing checkout (SPA)
• we expose a new page inside checkout-app which just imports the checkout (SPA) component and renders it

.
├── apps
│   ├── checkout   ⬅ we can keep this for backward compatibility
│   │   └── (…) just import packages/checkout-spa and render
│   └── checkout-saleor-app   ⬅ rename to make it clearer
│       ├── README.md
│       ├── backend
│       ├── frontend
│       ├── (…) existing checkout-saleor-app folders
│       └── pages
│           ├── (…) existing checkout-saleor-app pages
│           └── checkout-spa.tsx   ⬅ just import packages/checkout-spa and render
└── packages
    └── checkout-spa
        └── (…) existing checkout SPA component exported as a package

[witoszekdev]

@lkostrowski I don't think this was really formalized, but since every other Saleor app is using this naming convention I think we should stick to it 😄

[lkostrowski]

We did have this discussion on slack with some people, I just look for a way how to formalize it.

@patrys what do you think?

[patrys]

I agree that "app" on its own is meaningless, but "Saleor App" is a particular type of a Saleor extension as described by Saleor docs. I'd stick to the saleor-app-* or *-saleor-app convention (whichever makes more sense).

[lkostrowski]

Okay, since I'm stepping in from marketplace perspective I will formalize saleor-app-* convention on this side.
https://app.clickup.com/t/2na140k

@mmiszy can you update RFC/PR to follow this pattern?

[typeofweb]

Yes! I was planning to do that just before merging to avoid conflicts with the main branch.

[typeofweb]

PR, posting for transparency, it's work in progress: #146

[typeofweb]

Merged.