Open levino opened 8 months ago
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.
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.
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:
https://registry.jsonresume.org/thomasdavis?theme=kendall
with a route handler (or probably just keep the current set up).I see no other way.
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)
https://github.com/adamhl8/jsonresume-theme-cappuccino
@adamhl8 has done a great approach to getting a react theme rendering, I need to investigate it further
I've also been working on packages/jsonresume-theme-professional
which is react based and works nicely with the registry https://registry.jsonresume.org/thomasdavis?theme=professional
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
andfs.readFileSync
has been shown to be problematic. To this end we also need to decide on a strategy.I recommend that we continue to forbid the usage of the
fs
package in our codebase altogether (expect for build tools of course). Usingfs
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 likeimport
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.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 ourpnpm-lock.json
and with that use the new version that reintroduces the usage offs
, 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 offs
and then fails" but rather a way to reproduce our issues in the deployment with vercel during the run of ourtest:e2e
script.