Add instructions / conditions for themes

[levino]

I put in this issue to discuss the topic and remember it for the future. Will also do some work on it, when I find the time, but first lets discuss a bit.

We should add a Readme / docs section, describing conditions for themes. One important aspect is the fact that the usage of nodes fs.readFile and fs.readFileSync has been shown to be problematic. To this end we also need to decide on a strategy.

1. I recommend that we continue to forbid the usage of the fs package in our codebase altogether (expect for build tools of course). Using fs during runtime will always be problematic. One fact is of course, that any such code will not work in the browser or other environments but nodejs. I think it is never a good idea to runtime read a file that is part of the source code. Runtime reads of files should rather be for files that were created by the same code shortly before like some intermediary result file for image generation or something in order to keep memory low or so. There are good reasons to use bundlers for using .css or other non-JS-files in js codebases and these bundlers are very good at managing the dependencies and packaging all necessary files into js files in order for the code to run in the browser or wherever there is JavaScript.
   I did some work on this approach. I have an open PR where I use vite to bundle a css file into my output js file to then be able to render an html string with appropriate css inlined. It turned out that vite has this handy !inline postfix when importing files, which was exactly what I needed in my case. The file itself keeps its .css ending and linting and formatting works nicely. The only "downside" is, that the code has to be transpiled and bundled, before it can be used. As it is TypeScript and also uses some ECMA stuff like import that has to be done anyhow, so I think the downside is limited. This approach will basically do the "readFile" during build and not on runtime which is just way safer.
   In order for maintainers of third party themes to make their code compatible, we should have some example code ready. Of course this approach will also work with handlebars etc. if need be, even though I think handlebars is a bad technology, especially for our project because it tends to mess with the global variables.

2. Right now some maintainers have "fixed" their code so it does not use nodes fs any more. That is a workaround for the time being but sooner or later, theses fixes will be rolled back because people forgot what the problem about it was. If we then delete our pnpm-lock.json and with that use the new version that reintroduces the usage of fs, our code will break and we might not even see it during the reviewing of the PR. In order to prevent this, we need to think about regression tests that will fail if these bugs are reintroduced. However I would not prefer to have some kind of test that "looks for the usage of fs and then fails" but rather a way to reproduce our issues in the deployment with vercel during the run of our test:e2e script.

[levino]

On 2: I have just tried it out: If I make a production build of the registry and start that with pnpm start, I can reproduce the readFile error (I have to either provide a GITHUB_TOKEN for this or comment out the production check for it in order for it to work, so we will have to see how to go about that one, but that is fixable). So if we run our test:e2e script against a prod build, potentially by rendering one resume with all supported themes, we should have a nice regression test.

@thomasdavis I think this is also proof that the problem does neither come from turbo nor vercel but, if one can call it a "problem", then is stems from nextjs and the way nextjs bundles the code for production.

[thomasdavis]

I love the reasoning, and now agree, let's not promote the usage of fs going forward.

I like your approach, and the example I started on packages/jsonresume-theme-standard also works well.

I will try make time to take a stab at some documentation that covers both approaches.

[levino]

Regarding the changes in #92: I think it makes no sense to implement the functionality to "render a json from github as a resume using a user specified theme" as a page. It should be an api route / route handler. It should receive the path and the searchParams and then return the string generated by the theme. The challenge is that usually the themes produce multiple files: At least one .html file and one .css file. How they produce them varies from theme to theme. Ideally all the themes would just produce one string which includes all the html and css statements / code which is necessary to render the resume with the appropriate styling. I also think that it is no really possible or at least very difficult to support themes which produce multiple files because we would have to handle multiple requests. If we want to support this, we would need to discuss some kind of API first, because we basically would have to implement on endpoint which generates the .html file and another which generates the .css file (and the latter must be linked in the .html file by the theme correctly).

So what I suggest is:

• implement https://registry.jsonresume.org/thomasdavis?theme=kendall with a route handler (or probably just keep the current set up).
• require themes to produce on string that includes all necessary markup to render the page.
• remove support for all themes that currently do not comply with this API until they are fixed by the maintainers.

I see no other way.

[thomasdavis]

I suppose it is a question of whether we should promote themes with "side effects" or not, being fs or http.

I think I've switched back to the idea that we should encourage people to have pure functions when building themes. (it also has glaringly bad security problems).

Alright, let's not encourage that behavior. Peoples repositories can do whatever they want, but if they want to participate and be supported in the "official" JSON Resume, themes should be pure functions.

---

That aside, on the question of what I am going to call assets, because you could have css/images etc

I was building a cousin project called https://github.com/jsonblog/jsonblog-generator-boilerplate

Where you essentially return an array of objects that are file paths and base64 contents

[{
  "filename": "example.html",
  "content": "<!DOCTYPE html>\n<html>\n<head>\n    <title>Example</title>\n</head>\n<body>\n    <h1>Hello, World!</h1>\n</body>\n</html>",
  "encoding": "utf-8"
},
{
  "filename": "images/example.jpg",
  "content": "/9j/4AAQSkZJRgABAQEAAAAAAAD/4QB6RXhpZgAATU0AKgAAAAgAAkAAAAAAAABcAAIAAAAQAAAAYgAAABABAAIAAAAUAAAAngEAAwAAAAEAABAAADIBAABTAAAAAQAAADIBAgAUAAAAZAAAAGmHBAABAAAAqAAAAPkAAACWgAAAfQAAAKAAAACcAAAH5AAABWgAASUkAAAEAAADIAAD/2wBDAAQDAwQDAwQEBAQFBAQFBgoHBgYGBgYICQoJCgkJCQsLCwsMDAwMDAwODAwOEBAQEBAQFBAQFBQUFBQUF....", // Base64 content
  "encoding": "base64"
}]

In this setup, you are essentially outputting a public folder, and we could say that themes need to return an index.html or something.

And then for backwards compatibly, if we detect the return of render is a string, we assume it's html and render as we usually do, but if it is an array, we think it is the new system, and write the files, and serve.

Doesn't make super sense in a function based setup, so will think on that a bit more. (with a good caching strategy, this should be easily doable without being wasteful)