Closed thomasdavis closed 1 month ago
Latest commit: c94d41e9baafc9ac94038e66941313e403cc5c56
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
jsonresume-org-homepage2 | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | May 9, 2024 2:25pm |
jsonresume-org-registry | ❌ Failed (Inspect) | May 9, 2024 2:25pm |
The recent updates focus on enhancing the functionality of a Next.js application for generating and handling resumes. Modifications include adjustments to webpack configurations, API route changes, and the introduction of new files for fetching, validating, and formatting resumes based on GitHub gists. Additionally, there's simplification in theme exports and an expansion in handling different file types and themes for resume formatting.
File Path | Change Summary |
---|---|
apps/registry/next.config.js |
Updated webpack configuration to handle fs module and set webpack5: true . API route adjustment. |
apps/registry/pages/[payload].js |
Added functions for static props, paths, and initial props in resume handling. |
apps/registry/pages/api/... |
Introduced files for resume formatting and validation. Enhanced theme and formatter handling. |
packages/.../index.js |
Simplified theme export in jsonresume-theme-spartacus . |
[!WARNING]
Review ran into problems
Problems (1)
* Git: Failed to clone repository. Please contact CodeRabbit support.
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
got stuck on this https://github.com/vercel/next.js/discussions/17269
PR in progress, only file worth checking out is apps/registry/pages/[payload].js
-1 nextjs
What kind of URLs would you like to prerender?
I just saw: You are using the pages
router which is outdated. I suggest to use exclusively the app
router. Then there also is no getStaticProps
.
Even if you are using getStaticProps
you cannot possibly want to generate a page for each possible username and theme. I think you should just server side render these pages.
With the app router you can access the searchParams
prop in the render function.
Moving to the app
router makes sense. I will migrate it all there tomorrow.
Been trying to furiously refactor code because I have some spare time at the moment.
The end goal of this PR is to get the registry template/theme rendering working inside of pages/
and then any extensions for resume.json
, resume.yaml
etc outputted in the api
folder.
Which allows the templates to use nodejs libraries such as fs
, and then to logically move pure functional lib
like functions outside of the api folder.
With next.js you are in serverless / lambda land. There is no file system. Using fs
will never work properly because the code will be bundled and the bundler does not understand that the files which you will later load via fs
are to be bundled as well. My recommendation: Just drop support for all the templates which use fs
under the hood. I think this will never work properly.
feel free to commit any code you want to my pr's, it don't bother me, i'm sure we both have adequate git-fu
I know we've talked about the fs
support before, and I lean towards what your thinking. Will share my reflections tomorrow.
It is a question of requirements. I cannot decide that for you. I am just saying you would make your life easier if you lowered the bar. If I can be of help with some code, let me know and I will see what I can do, but I will not support fs
. It is just too painful imo.
Closing in favor of https://github.com/jsonresume/jsonresume.org/issues/58
Keep the branch alive for now
mostly just to support
fs
again. (which took way too long to figure out)but the abstractions will make more sense nontheless
not ready for review: pr'ing to show the line of thinking
lib
like functionality out of the api folderfs
works againfs
anyway (find relevant issue where @levino made a good point that it shouldn't be possible)