-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate to react router 7, typescript, and misc. refactoring #27
base: master
Are you sure you want to change the base?
Conversation
@sorokya did you get a chance to look at this yet? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. I still need to test it locally but changes generally look good. I think there's a few comments from your self review that need action. I left a few comments and questions as well.
I'll test this locally some time in the next week 😊
import { glob } from 'glob'; | ||
import { parseMarkdown } from './utils/parse-markdown'; | ||
|
||
type Content = Awaited<ReturnType<typeof parseMarkdown>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Awaited a built in typescript type to describe an "awaited" value? Neat. Not sure why it's needed though really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we lose the Awaited
, the type becomes wrapped in Promise<>
, feels less "clean".
} | ||
|
||
const fileContents = fs.readFileSync(DATA_FILE_PATH, { encoding: 'utf8' }); | ||
const json = JSON.parse(fileContents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't fileContents be json and this variable be named something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave it as it is. fs.readFileSync
outputs a string, which we parse as JSON in the second step.
@sorokya Anything else you'd like to see here before we get this merged in? |
Major changes:
Minor:
Pre-release steps (in dev and in prod):
bun upgrade
)Known issues:
rm -rf .react-router && bun run dev
to generate types again.