Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork254
refactor: switch to React Router#1115
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3d98702 to71c2897Comparecloudflare-workers-and-pagesbot commentedSep 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Deploying rescript-lang-org with |
| Latest commit: | 2074165 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6d713ec8.rescript-lang.pages.dev |
| Branch Preview URL: | https://vlk-v12-react-router.rescript-lang.pages.dev |
…ript-lang/rescript-lang.org into vlk-v12-react-router
…ript-lang/rescript-lang.org into vlk-v12-react-router
…nto vlk-v12-react-router
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.
Pull Request Overview
This PR migrates the ReScript documentation website from Next.js to React Router. The migration involves removing Next.js-specific page files, updating the build configuration, and restructuring the application to use React Router v7's routing system.
Key Changes
- Replaced Next.js routing with React Router v7
- Updated build tooling from Next.js/webpack to Vite
- Migrated MDX processing from Next.js MDX to react-router-mdx
- Updated package dependencies and removed Next.js-specific configurations
Reviewed Changes
Copilot reviewed 165 out of 1093 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated dependencies to replace Next.js with React Router v7 and related packages; modified build scripts |
| next.config.mjs | Removed Next.js configuration file (entire file deleted) |
| jsconfig.json | Removed jsconfig.json file (entire file deleted) |
| generate-route-types.mjs | Added new script for generating route types from MDX files |
| pages/_app.js | Removed Next.js app wrapper file |
| pages/blog.js | Removed Next.js blog page file |
| pages/blog/[slug].js | Removed Next.js dynamic blog post route file |
| pages/blog/archived.js | Removed Next.js archived blog page file |
| pages/brand.mdx | Removed brand assets MDX page |
| pages/blogpost-guide.mdx | Removed blogpost guide MDX page |
| pages/docs/manual/v10.0.0/api.mdx | Removed API introduction page |
| pages/docs/manual/v10.0.0/api/belt.mdx | Removed Belt standard library documentation |
| compilers/dummy/Dummy.res | Changed Console.log to Console.debug |
| docs/react/*.mdx | Updated React documentation examples to remove ReactDOM.Style.make API usage and fix inline styles |
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
jderochervlk commentedNov 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| name, | ||
| items: Array.map(values, ((href, value)) => { | ||
| // TODO: this probably doesn't work as expected | ||
| NavItem.name: value["title"], | ||
| href, | ||
| href: ((value["headers"]->Array.getUnsafe(0))["href"] :> string), | ||
| }), |
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.
Sidebar items no longer navigate to docs
The sidebar categories build each item’shref from the first header in the document (value["headers"]->Array.getUnsafe(0)["href"]). This produces anchor‑only links such as#introduction instead of the document path (href from the tuple). When rendered, every sidebar entry points to an in‑page anchor on the currently loaded document, so clicking a different entry no longer navigates to the target document. The previous implementation used the page path and worked; the new logic breaks doc navigation for the entire sidebar.
Useful? React with 👍 / 👎.
|
| @@ -0,0 +1,17 @@ | |||
| letloader=async ()=> { | |||
| letprops=awaitPackages.getStaticProps() | |||
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.
Nit: this binding is a bit unnecessary
| - Make sure to give a descriptive package name. We usually use`rescript-[name-of-js-lib]` for packages that bind to a specific JS library on npm. | ||
| - Use names that are self explanatory (no weird marketing terms / fantasy words if possible). | ||
| - Use names that are self explanatory (no weird marketing terms /pages/docs/guidelines fantasy words if possible). |
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.
Search & Replace issue?
| -[bun](https://bun.sh/) | ||
| -[deno](http://deno.com/) | ||
| - Configure`"nodeModulesDir": "auto"` in`deno.json` | ||
| <divclass="install-list"> |
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 this temporary or why is it needed?
| let child = list.children->Option.flatMap(children => children[0]) | ||
| switch (list.url, child) { | ||
| | (Some(url), Some(child)) => { | ||
| if child.value->String.includes("title:") { |
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.
this seems useless?
| @@ -576,6 +576,7 @@ let useCompilerManager = ( | |||
| } | |||
| ) | |||
| | Executing({state, jsCode}) => | |||
| () | |||
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.
leftover?
| @@ -546,14 +550,22 @@ let make = // props relevant for the react wrapper | |||
| ~keyMap=KeyMap.Default, | |||
| ~lineWrapping=false, | |||
| ): React.element => { | |||
| Console.debug("staring codemirror") | |||
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.
leftover?
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 not then it should be
"starting codemirror"
| @@ -0,0 +1,162 @@ | |||
| # rescript-lang.org | |||
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.
why is this file in src/components now?
| let toctree = tree->Dict.get(moduleName) | ||
| Ok({module_, toctree: Obj.magic({name: "root", path: [], children: []})}) | ||
| // let toctree = tree->Dict.get(moduleName) |
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.
leftover?
| // The path isn't included when we are rending a post vs listing them | ||
| let archived = try { | ||
| mdx.path->String.includes("/archived/") | ||
| } catch { |
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 this needed? String.includes usually only throws when you give it a regex?
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.
ooops I missclicked
| @@ -0,0 +1,177 @@ | |||
| type t = [ | |||
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.
Lovely!
| require("plugins/cm-reason-mode"); | ||
| } | ||
| `) | ||
| // CodeMirror 6 - no longer need these requires since we import directly in the component |
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 guess we can just remove this comment then.
| ->Option.getOr(CodeMirror.KeyMap.Default) | ||
| CodeMirror.KeyMap.Default | ||
| // Dom.Storage2.localStorage |
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.
remove?
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.
or restore
| @@ -0,0 +1 @@ | |||
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.
empty file
| entries: [], | ||
| } | ||
| type c = { |
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.
what's c? content? Name could be a bit more descriptive.
| @@ -0,0 +1,1332 @@ | |||
| [ | |||
| { | |||
| "path":"/home/josh/Dev/rescript-lang.org/_blogposts/2025-04-11-introducing-unified-operators.mdx", | |||
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.
paths are wrong
Okay done with the full review. I saw a lot of TODOs still that I think should be addressed before the merge, but I only commented where I saw an Issue and there was no TODO already. |
Uh oh!
There was an error while loading.Please reload this page.