Internal refactoring

[DCzajkowski]

While adapting Outstatic to our needs, we have found, the code is written with a lot of indirection, assumptions, and coupling. While that's ok for an early prototype, if we want to introduce more customizability/plugins, this should be thought about and worked on.

I am creating this discussion to keep track of which refactorings @avitorio is comfortable with and which of those should be worked on.

I am creating a thread for every thing I though of when working on Outstatic. Please PR only those refactorings @avitorio has agreed to.

[DCzajkowski]

There are many assumptions in the code. One of the examples:

const image = resolvedImage?.replace('/images/', '/api/outstatic/images/')

assumes images are in the images/ folder. This is true only if createCommitInput puts them there. /api/images.handler also utilizes this assumption.

We need to turn those assumptions into facts ie. extract this duplicated information into constants.

[thecodedrift]

WRT the missing constants, can we build a list either here or in an issue @DCzajkowski? This way we've got a checklist we can knock out quickly.

[avitorio]

I agree with @jakobo that allowing for custom paths for outstatic is a feature on its own, due to the possible implications of this change in how other parts of the system functions...

I've created an issue for the possible constant upgrades.

I know this will sound like newbie question, but where in the project would be the best place to keep these constants? Is there any kind of convention that someone here would like to suggest?

[thecodedrift]

Ping for @DCzajkowski - did you want to take on the constants work? If not, the earliest I can get to it is 2022-11-22, though a list would speed up the effort.

RE convention: I'd just make a constants.ts file.

[DCzajkowski]

I'll take that 👍

[DCzajkowski]

#67

[DCzajkowski]

Having GitHub's GraphQL calls on the client causes a flash of content and lots of rerenders, that are especially annoying as the GitHub API is slow af. Move all API calls to the server?

[thecodedrift]

I can weigh in on this one with some thoughts. For context, I've written the current test infra for our GraphQL stuff and the initial collections query test.

For readers: If following the example from https://github.com/avitorio/outstatic/blob/main/examples/blog/src/pages/posts/%5Bslug%5D.tsx, there shouldn't be a flash of content since everything is completed within getStaticProps (gsp). Ideally on the production site, everything comes from the filesystem. However, if you've enabled fallbacks in 'getStaticPaths`, there might be a delay on initial render if next hasn't cached the static version of the page yet. In next, that'll affect the time to first byte.

For editors: This one's more tricky. Apollo's hydration strategy kinda sucks (example post), and I agree that GraphQL's api is slow in the best case. We should do as much as we can inside of getServerSideProps on the editor, with SWR style hydration. In editor mode, we always use the GitHub API, so fixing this will improve the editing experience significantly.

As for how to fix it, personally I think builder patterns such as typed-graphql-builder + urql with the built-in next.js support can both improve maintainability of our GitHub calls as well as reduce some of the testing surface area. Specifically

• Builder patterns will surface GitHub API errors at the TypeScript level without requiring Jest tests to confirm behavior
• Urql has strong next.js support, and there is already a discussion about server component support

Editor improvement would be (1) switch core technologies for the small number of gql queries we make, (2) add the types generator for the github schema to the build process to ensure build fail during tsc if something changes (3) add queries and extractData into getServerSideProps.

All that said, the flash is annoying and should be fixed, but its probably less pressing than some of the lower hanging fruits such as constants in use or un-index.tsxing everything.

[avitorio]

Hey,
I apologize for not being so active here. I am in the middle of renovating an apartment with a short deadline to move in, so all my time outside of my day job has been used towards painting and other work.

Thank you for the suggestions for improvement.

I have thought about moving everything on the dashboard to ssr before but the way the current system is set up, is this: we only have one place with ssr in the library which is pages/index.tsx. All other pages are components. This is the way I found to make setup/installation easy by just adding a couple of import statements on Next.js, ie: /outstatic and /api/outstatic.

I think ssr would be ideal, but we need to consider how to make it work with just importing OstSSP.

I hope this makes sense. I am on mobile and might have mistyped some of it.

[thecodedrift]

Yeah, it's definitely doable. We just need to return a discriminated union to tell the props apart + use req.params to figure out from the route which data calls to make for the page in question.

[avitorio]

Yes, I'm sure it's possible. When I tried to start working on it I thought the ssr logic would get too bloated, considering all the possible paths. But this can be organised in a way that makes sense. The user experience would be greatly improved by it.

Another aspect I think we need to consider is caching. Ideally we should always get the latest data when a page loads, to avoid people working on old versions of documents, etc...

[avitorio]

As a side note, we could show users an animated loading skeleton in some parts of the dashboard. My concern is that with SSR we may not experience the flash of content, but pages may take a bit longer to load.

[DCzajkowski]

Next 13 introduces Server Components. Should we look into those?

[avitorio]

I haven't looked into this but it might be the answer to my previous comment about only having one entry point for the dashboard (OstSSP). Right now all the ssr logic is handled there, if we can move it to the component level this could help us, but I wonder if we'd still get a delay before seeing data.

[DCzajkowski]

In Next 12 this would be a problem everywhere: slow getServerSideProps causes no loading indicator and data comes in late.
In Next 13 and app/ directory, this is solved with the loading.tsx file.

If we want to preserve one entry file this is still possible, but the loading states won't be as easily customizable, I think. Haven't looked into that too much, yet.

[avitorio]

I really need to find some time to play with these new Next.js 13 features...

[DCzajkowski]

Having every file called index.ts is hard to work on IMO. I would suggest renaming files:

// change this
DocumentSettings/
  index.tsx
  test.tsx

// to this
DocumentSettings.tsx
DocumentSettings.test.tsx

[avitorio]

I think this comes down to personal preference. I like to work with index.tsx, especially for imports, but I am not married to this convention. We can handle that through an index file that handles all the exports, as we have under the components folder. I do see the advantages, especially when looking at IDE tabs... If you and @jakobo prefer to work in this fashion I'm ok with making the switch. 👍🏼

[DCzajkowski]

Some files are not ran through Prettier. I would move Prettier to ESLint for easier setup. This requires to run just ESLint, to both format and lint. It's easier to setup and to maintain (as all stylistic rules, file whitelists, etc. are in just one tool/config).

[DCzajkowski]

I had bad experience with hooks on commits, but that was a few years back and badly implemented.

If it's only stylistic and very fast to run (not noticable much), I think I am ok with it.

---

How does this work with ESLint rules that require type information? In my experience those are very useful, but also very slow. Can they even be ran on individual files?

[thecodedrift]

How does this work with ESLint rules that require type information?

You run those rules only on CI/CD, because they often do require a check of all files. For another project I contribute to, we do a tsc --noEmit check on pre-commit, but I agree that slows down the commit hooks enough to be a dealbreaker if the goal is fast & frequent commits.

edit the best answer I think is a branch where you can try out the hooks and see if it's providing the value we want. I'll put together a branch with the implementation first so we can talk about the code. 🏗️

[thecodedrift]

Here's the branch with the proposed change for how lint-staged on commit might work. This way others can play around with the idea too.

main...jakobo:outstatic:jakobo/d/25/lint

And here's the timing information I was able to generate. I ran it against the files that would be required to enable lint-staged +husky (how meta) which is 8 files. Locally it timed at ~.7s, while the GitHub workflow took ~10s to return not including waiting for the job to start.

time pnpm run lint-staged

> @ lint-staged /home/jakobo/code/outstatic
> lint-staged

✔ Preparing lint-staged...
✔ Hiding unstaged changes to partially staged files...
✔ Running tasks for staged files...
✔ Applying modifications from tasks...
✔ Restoring unstaged changes to partially staged files...
✔ Cleaning up temporary files...

pnpm run lint-staged  0.71s user 0.13s system 118% cpu 1.041 total

As measured from a GitHub Workflow

Set up job                      1s
Run actions/checkout@v3         1s
Run pnpm/[email protected]    2s
Run actions/setup-node@v3       2s
Run pnpm install                3s
Run pnpm lint                   1s

I'd recommend checks like tsc --noEmit be done at the CI/CD layer, as vscode is already very good about warning you when there's type issues, so this branch does not include the TSC items (it's commented in the prettierrc should we ever want to enable it).

For locally typechecking the project, I'd recommend we continue using the existing turbo run typecheck which ensures we have dependencies built and .d.ts files generated when required.

[avitorio]

Hey @jakobo, I just tested this locally and it's working as it should. If you want to create a branch and reference #63 on it I'll review it more in-depth and can merge it to canary if everything looks good.

[thecodedrift]

PR in #66

[avitorio]

Hi @DCzajkowski,
The thought of refactoring the codebase is something that's been on the back of my mind.
What we currently have is definitely just an early prototype, proof-of-concept type of thing.
Now that it's validated in terms of interest, a refactoring effort with a long-term vision is definitely needed.
Thank you for taking the initiative of bringing up those. I'll review them and give my thoughts asap.

[DCzajkowski]

Great!

[ahaasler]

Hi, i'm going to be referencing TinaCMS a bit because I had experience using that CMS on its baby steps, please bear with me.

One of the first problems was related to how to handle all content with FS calls (that could be used via GitHub API) for very large projects, I think that the solution by @jakobo in #53 is the way to go and should improve performance in multiple scenarios and enable new use cases, like i18n.

I think we should decouple the editor/UI part of Outstatic from the content layer and authentication to avoid vendor Lock-in. Right now using GitHub is really easy and great for Outstatic to get known, but not everyone is able or willing to use GitHub. I'm thinking about defining well thought interfaces so the content part of the GitHub API is easily replaced with other implementations (GitLab, Sourcehut, Redis cache, private client implementation). With this I don't mean to completely change the way Outstatic works with the content structure in the FS or limit that in any way, but to interface those calls for what Outstatic really wants, and be able to provide a different implementation, for example, store TEXT with METADATA could be easily be done on Redis, with the Matadata DB also being stored on Redis. The same for listing collections, etc.

TinaCMS started with this approach but abandoned it years ago in favor of using TinaCloud and getting control of the backend. They just announced pricing plans and people are not happy to say the least.

This plugable approach to the content backend may be necessary for some clients, since some sites may reach GitHub rate limits. Having options is a really nice ofering for open source projects.

But GitHub is not used only for content, it's also used for authentication, which in the current state is really helpful for an easy set up (also development). But we can't count for every content backend to also implement auth since it may not make sense. We should decouple auth from content.

[avitorio]

These are good suggestions.

I'm up for decoupling content and UI to be able to offer localized dashboard experiences in the future.

As for the others, I think this is where we will end up going eventually: Multiple storage and authentication options.

In my opinion that would require the use of dependency inversion wherever possible and create adapters for authentication and storage.

But before we go there, I want to make sure some other aspects of the system are well established, such as tests, CI/CD and developer documentation. I'll try to create a few issues and a roadmap for more of these base features and then we can move on to broadening our auth and data source support.

[Thisisjuke]

Dealing with monorepos

Context

Hi everyone, I am benchmarking things, and I need something working like a "standalone" MDX editor.
We already have a benchmark around : https://github.com/sanity-io/sanity (we really like their build-in preview using NextJS preview).
→ We think that's their studio is too complex for our use-case + not self-hosted.

Problem 1 : untimely commit / push

Now, our application is looking like that, it's a turborepo monorepo:

client_monorepo/
├─ apps/
│  ├─ documentation/
│  ├─ studio/
│  ├─ website/
├─ node_modules/
├─ packages/
│  ├─ config/
│  ├─ ui/
├─ package.json

→ Studio can be outstatic here.

When we are editing things during local development, we don't want commit / push all the monorepo at each edit. Studio will be used in local to write documentation MD, and will be used in production for blog posts.
We don't want things to be pushed during development, because each push trigger all the build of all packages / applications.

Problem 2 ; We have our own CI/CD, we don't want to work with Outstatic default Github workflow

❌ Variable OST_GITHUB_ID is missing!
❌ Variable OST_GITHUB_SECRET is missing!
❌ Variable OST_TOKEN_SECRET is missing!
❌ Variable OST_REPO_SLUG is missing!

Can we disable this and just keep the changes in the MDX: things will be handled by our own push / webhooks ?
I see some similarity with: #54 (comment)

Thx for your time ✌️ !

[avitorio]

Hey @Thisisjuke, as of now there are no plans to turn Outstatic into a standalone MDX editor. One of our core ideas is having the remote repository as the source of truth for content, so the idea of committing locally is not something we envision doing any time soon.

That being said, I think Problem 1 can be mitigated by pointing your OST_REPO_SLUG and OST_REPO_BRANCH to different repos/branches locally vs. production. So you'd have a 'documentation' branch for example, and when using outstatic locally you'd only push to that branch. Then you'd be able to merge the changes as you see fit. I know this is not ideal, but perhaps there's a way to achieve something closer to what you want by playing with Outstatic's environment variables.

As for Problem 2, I fail to see how these environment variables are conflicting with your CI/CD. These variables are only used if you are using the Dashboard to edit content...

Thank you for your interest in the project. I hope you can make it work with with the way things are currently done on our end.

[DCzajkowski]

Eliminate mutations and "clever" code. Mutations usually are a cause of bugs. My suggestion is to replace all mutating code with non-mutating.

There are places where mutation might be required, but I think this is very rare.

In some places there is also "clever", not-very-clear-what-it-does code. I think that should be eliminated to make the code easier to understand.

---

As an example I will take first six lines of mergeMdMeta function:

const meta = Object.entries(
  (({ content, publishedAt, ...meta }) => meta)(data)
)

if (data.publishedAt) {
  meta.push(['publishedAt', data.publishedAt.toISOString()])
}

That could be rewritten to:

const dataWithPublishedAt = {
  ...data,
  ...(data.publishedAt ? { publishedAt: data.publishedAt.toISOString() } : {})
}
const meta = Object.entries(omit(dataWithPublishedAt, ['content']))

---

BTW, in this code example, data.publishedAt is typed as Date, so either the if statement is unnecessary or the type is wrong.

[avitorio]

I agree these look much better. We may not need the conditional publishedAt and can just cast it to string. I think that if statement was a check I added to avoid a typescript error or something... Thanks for pointing it out!

[DCzajkowski]

What is the reason for turning off react/jsx-keys rule @avitorio? I think it should be turned back on

[avitorio]

No reason, I think we can remove it. That's left over code from the turborepo boilerplate I used. Good catch!

[DCzajkowski]

I've just noticed: ESLint is not set up for packages/outstatic. When you run pnpm run lint, it runs only on apps/web.

[avitorio]

I noticed that as well when looking for the react/jsx-keys config 😅 - working on it now.