We have a heuristic that only does a full route update when path params change. This is to make it easy to do more "surgical" updates of query params, where only what you're interested in is fetched/refetched. But, this becomes confusing because it means using links that just change query params does not actually work unless they also change at least some part of the path. This goes against the goal of advocating for thinking about "params" rather than path vs query params.

I think we can support both cases quite easily, so let's find a good way to do that, maintaining "links always work" while also having the option to do more surgical updates to query params that does not trigger a full route re-render.

cc @Kingdutch , we should also cover the "click on the same link and have it refresh the current route" case here.

We can validate at build time that the patterns we piece together from the route structure will actually work, by simply matching on them via react-router as we extract them. Do that, and add errors if they fail.

Follow-up to #86

This is the biggest size culprit. Find a way to reduce its size.

Problem

We have some code which deals with Component loading, which makes sense because it originates from lazy loading components. However, with our new and improved set-up there's nothing that mandates it's specifically a Component and we're more dealing with Scripts's where the underlying preloading of components already keeps track of what scripts are needed.

Solution

Update the API to refer to Script instead of Component so it's clearer for consumers that this is generic and not specialized.

Logic exists in both the RescriptRelayVitePlugin and in lookup.mjs. Figure out why and dry up.

JSONC allow us to add comments, which is great for our purposes. Should document that.

This ensures that applications no longer need to define it themselves.

The bindings should also move into the router, but the difficulty there is that they rely on a stream type which is currently tied to NodeJs.

We should check if the stream also works for things like Cloudflare workers (or other serverless environments) and either a) provide a TransformStream (an example of this vs the NodeJs writable stream can be found in the React 18 working group discussions) b) if Writable streams are supported on those platforms too we could use the same class but have to make the type abstract

Fix #84

It's easy to create a fallback page for when a page is not found. However, the result of this is that when this catch-all route is matched the status code is still 200 rather than 404. Especially when a browser tries to load some kind of asset this may be problematic.

Follow-up to #86

Extend the types of things that we can preload. MDN contains a list of valid values for as that we probably want to try out with e.g. Vite to see how it exposes the data to ensure we can get it into our manifest and preload it with the right time. We may decide that we want to replace the empty arrays (easy to use) with nullable objects (smaller to transfer) if we have many different types in our manifest.

RelayRouter__ + top level RelayRouter.

In #78 I've changed <script type="module" src="x"/> insertions to a dynamic import which is slightly more succinct and has the same effect.

The drawback is that Vite now knows we're loading something but can't analyse it and will complain

 2:59:05 PM [vite] warning: 
/rescript-relay-router/packages/rescript-relay-router-core/src/RelayRouter__Asse
 25 |        return ;
 26 |      } else {
 27 |        import(asset._0.__$rescriptChunkName__);
    |               ^
 28 |        return ;
 29 |      }
 The above dynamic import cannot be analyzed by vite.
 See https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations for supported dynamic import formats. If this is intended to be left as-is, you can use the /* @vite-ignore */ comment inside the import() call to suppress this warning.
 @vite-ignore */ comment inside the import() call to suppress this warning.
 
   Plugin: vite:import-analysis
   File: /Users/alexander/Projects/rescript-relay-router/packages/rescript-relay-router-core/src/RelayRouter__AssetPreloader.mjs

I think we have three options to get rid of this warning

1. The easy thing would be to revert the import but that actually hides from Vite that we're actually importing something.
2. Another option is to generate an actual import function that we could call here for each Component (which I think we removed in the past 😅 ) rather than passing a variable to import. This has the upside that Vite may be able to do smart things with our dynamic imports
3. The final option is to create a codemod and add the suggested /* @vite-ignore */ string to our import in this case if we think Vite optimizing our dynamic import would cause issues.

React has a good set of lints for things like #26

By creating a test step in this repository that runs ESLint with those lints against our compiled JavaScript code we can make sure that our Rescript code also adheres to those standards.

Similarly in the following code (until line 274) it would probably tell us two things:

1. React hooks may not be called conditionally (use*Effect will not be called on the server so it's safe to keep in server side code)
2. The useLayoutEffect is missing the router and routeEntry dependencies.

Will make it easier to find what you're looking for. Essentially, as skeletons for route renderers are emitted, emit them in the corresponding folders.

# Everything is flat now
- routes
  Root__Dashboard__Organization__Details_route_renderer.res

# Can do it like this
- routes
  - Dashboard
    - Organization
      - Details
         Root__Dashboard__Organization__Details_route_renderer.res

There's a few things relying on the current flat structure, but I think it'll be easy changing.

Biggest problem is I'll need a migration script for this for my own codebase that has enough routes that doing that by hand would be majorly painful...

Up for debate what the best pattern is here.

I've noticed that the Routes..Route module won't update in case there are other errors in the Rescript project. This is quite problematic in case the error in the project is a type mismatched caused by code in that module (e.g. the query parameters were incorrectly typed). To fix this the user must first remove the usage of the query parameter to make the project compile and then rebuild the project.

It's currently easy to change query parameters, but it'd be cool to have something that allows updating path parameters easily as well.

An example use case:
Imagine you have a backoffice where you can navigate to different pages to administer organizations. The organization slug is a path parameter in the URL. The page also has a search bar at the top, where you can search for organizations and move to their backoffice.

Now, imagine you want to stay at the current page when searching in that bar, just switch out what org you're looking at. This is pretty painful to do today. But, if we have something like Routes.Backoffice.Dashboard.updatePathParams(~slug=newSlug) then that would be very easy instead.

cc @Kingdutch , what do you think?

A way to just paste a link to any page in your app, and via that navigate to the route renderers responsible for rendering that route.

Warning: usePreloadedQuery(): Expected preloadedQuery to not be disposed yet. This is because disposing the query marks it for future garbage collection, and as such query results may no longer be present in the Relay store. In the future, this will become a hard error. 
    at HomeRoot (http://localhost:3000/src/HomeRoot.bs.js:115:24)
    at make (http://localhost:3000/src/routes/Root__HomeRoot_route_renderer.bs.js:53:15)
    at RelayRouter$RouteComponent (http://localhost:3000/node_modules/.vite/deps/rescript-relay-router_src_RelayRouter_bs_js.js?v=119ac42f:217:16)
    at Suspense
    at section
    at div
    at Root (http://localhost:3000/src/Root.bs.js:114:24)
    at make (http://localhost:3000/src/routes/Root_route_renderer.bs.js:54:15)
    at RelayRouter$RouteComponent (http://localhost:3000/node_modules/.vite/deps/rescript-relay-router_src_RelayRouter_bs_js.js?v=119ac42f:217:16)
    at Suspense
    at RelayRouter$RouteRenderer (http://localhost:3000/node_modules/.vite/deps/rescript-relay-router_src_RelayRouter_bs_js.js?v=119ac42f:220:29)
    at ErrorBoundary (http://localhost:3000/node_modules/.vite/deps/@rescript_react_src_RescriptReactErrorBoundary_bs_js.js?v=119ac42f:12:16)
    at Suspense
    at RescriptRelay$Context$Provider (http://localhost:3000/node_modules/.vite/deps/rescript-relay_src_RescriptRelay_bs_js.js?v=119ac42f:2144:27)

Reproducible here: https://github.com/cometkim/rescript-relay-issue-tracker

• Forward (and apply) Relay errors via the stream
• Ensure fatal errors fall back to trying to client render

...more things? cc @Kingdutch

https://github.com/cometkim/rescript-vitest

As per Discord discussion :)

Not highly prioritized, but it'd be nice for the CLI to be able to dump a full JSON structure for the routes, that includes everything you'd need if you wanted to do something custom outside of the CLI.

As outlined in #67 the suspense boundary of the router has been removed. This provides the application developers the power (but also the responsibility) on what should be rendered before we start sending data to the client. This is mentioned in the React Docs briefly but desires further explanation.

Multipart currently isn't handled, mostly because it has been really hard to find a setup for fetching multipart responses that also work server side without issues. Once that's up and running it should essentially just be about setting the final flag correctly in the responses pushed via the stream.

I found at least one example where a non-optional parameter comes before an optional named parameter. This results in making the optional named parameter non-optional or requiring another unit argument.

I would propose to change this to

let isRouteActive = (~exact: bool=false, {pathname}: RelayRouter.Bindings.History.location): bool => {

This enables the following to work without errors:

Route.isRouteActive(location)
Route.isRouteActive(~exact=false, location)
location->Route.isRouteActive
location->Route.isRouteActive(~exact=false)

We can use this issue to find other places where ergonomics can be improved.

• Don't delete route renderers, rather report them as unused in diagnostics?

The router relies on some requestIdleCallback for some functionality. However, this is not supported by the latest versions of Safari on desktop and mobile.

We should use a fallback in case requestIdleCallback is not available.

A code action that adds a new route to a position, as in inserts the route object in its entirety, and maybe places the cursor at the correct place.

We changed how we did asset preloading which means the preload function is no longer in the router but now in user land. However, we didn't update the README in #55 which means that following the README no longer leads to a working application.

The following errors were encountered while updating to beta24

FAILED: src/entry.client.cmj

  We've found a bug for you!
  /Users/alexander/Projects/front-end/packages/app/src/entry.client.res:7:33-14:1

   5 │ 
   6 │ // Configure our router. Cleanup is unused on the client.
   7 │ let (_cleanup, routerContext) = RelayRouter.Router.make(
   8 │   ~routes=RouteDeclarations.make(
   . │ ...
  13 │   ~routerEnvironment=RelayRouter.RouterEnvironment.makeBrowserEnvironme
     │ nt(),
  14 │ )
  15 │ 
  16 │ @val external documentCookie : Js.Nullable.t<string> = "document.cookie
     │ "

  This call is missing an argument of type
  (~preloadAsset: RelayRouter.Types.preloadAssetFn)

rescript: [136/146] src/entry.server.cmj
FAILED: src/entry.server.cmj

  We've found a bug for you!
  /Users/alexander/Projects/front-end/packages/app/src/entry.server.res:12:34-19:3

  10 ┆ 
  11 ┆ // Configure our router.
  12 ┆ let (cleanup, routerContext) = RelayRouter.Router.make(
  13 ┆   ~routes=RouteDeclarations.make(
   . ┆ ...
  18 ┆   ~routerEnvironment=RelayRouter.RouterEnvironment.makeServerEnvironmen
     ┆ t(~initialUrl=url),
  19 ┆ )
  20 ┆ 
  21 ┆ Promise.make((resolve, reject) => {

  This call is missing an argument of type
  (~preloadAsset: RelayRouter.Types.preloadAssetFn)

Currently, each route renderer is code split, but the infra that loads each route is always emitted in one single file. This means that while we don't pay bundle costs for route renderers and how much code each route loads, we're still paying costs for every route we add, whether it's used or not.

I'd like to experiment with code splitting on route definition files. If this works out, it means we can replicate the "multiple entry points" pattern (like having a single app that has an admin section baked in for example) with very little bundle size cost. Which would be cool!

As discussed in #23 the history API is currently private to the router, though the API itself can be very useful for applications who need access to this data (e.g. for navigation or analytics).

This ticket proposes to move the history bindings to a standalone package in the repository that can be published to NPM so that other projects may use history without the router. To make consumption by projects using the router easy we can simple re-export the module to avoid projects using the router from having to depend on the history packages themselves.

#Problem
The RelayRouter RouteRenderer component currently renders a suspense boundary. This prevents applications from building up a minimum amount of loaded data before sending a UI to the client.

We rely on the onShellReady to start sending data to the client. React 18 considers the "shell" to be "anything that's not within a suspense boundary". This provides the UI engineer power to determine how fleshed out the app-shell should be and define a data-minimum.

Because the RouteRenderer renders a suspense boundary, no data loading (leveraging the router) can happen to produce an app shell. This prevents e.g. rendering at least a full header on the server side while allowing the rest of the content to stream in.

Proposal

Remove the renderFallback argument to RelayRouter.RouteRenderer and do not render a Suspense boundary in the router itself. This allows applications to define what data they want loaded before responding to the client.

Users who want to send something to the client without waiting for any data can still achieve this by defining the suspense boundary around the router themselves.

Replacing:

          <RelayRouter.RouteRenderer
            // Fallback renders whenever suspense bubbles all the way up here, to the router
            renderFallback={_ => React.string("Fallback...")}

            // This renders all the time, and when there's a pending navigation (pending via React concurrent mode), pending will be `true`
            renderPending={pending => pending ? React.string("Pending....") : React.null}
          />

with

        <React.Suspense fallback={React.string("Fallback...")}>
          <RelayRouter.RouteRenderer
            // This renders all the time, and when there's a pending navigation (pending via React concurrent mode), pending will be `true`
            renderPending={pending => pending ? React.string("Pending....") : React.null}
          />
        </React.Suspense>

ReplaySubject's are used to replay Relay data that was retrieved server side, client side. This is how shipping data from the server to the client is handled. However, we need to determine when a replay subject is cleaned up/removed (so it's not accidentally used again when we want fresh data from the API via the client), and this is slightly messy for various reasons.

This ticket writes down some thoughts what could be part of the public API. This is based on trying to use the router in an application.

Utils

RelayRouter__Utils implement hooks and helper functions that should probably all be available on RelayRouter itself. I think it may make sense to even skip the .Utils namespace here since they'll be commonly accessed.

I do notice that isRouteActive and useIsRouteActive are also on the individual route modules. Are they both supposed to be used or should only the route specific ones be accessed?

Bindings/History

RelayRouter__Utils.useLocation returns a RelayRouter__Bindings.History.location entry. This means that the bindings for History are needed to actually get anything from this value.

New functions

One thing I was trying to do was to create a login route that redirected to the current page. However, there's no easy way to assemble a path (including query string and anchor) for the current page from a location object.

Steps to reproduce

1. On master branch open the app in development mode
2. Go to /
3. Click "Todos", you're now on /todos
4. Hit the browser back button

Could not reproduce in production mode by running yarn build && yarn preview.

Error

Warning: usePreloadedQuery(): Expected preloadedQuery to not be disposed yet. This is because disposing the query marks it for future garbage collection, and as such query results may no longer be present in the Relay store. In the future, this will become a hard error. 
Layout@http://localhost:9999/src/components/Layout.mjs:101:18
RelayRouter$RouteComponent@http://localhost:9999/router/RelayRouter.mjs:178:3
RelayRouter$RouteRenderer@http://localhost:9999/router/RelayRouter.mjs:182:23
ErrorBoundary@http://localhost:9999/node_modules/.vite/deps/@rescript_react_src_RescriptReactErrorBoundary_mjs.js?v=0d390227:12:16
App
ErrorBoundary@http://localhost:9999/node_modules/.vite/deps/@rescript_react_src_RescriptReactErrorBoundary_mjs.js?v=0d390227:12:16
Suspense
RescriptRelay$Context$Provider@http://localhost:9999/node_modules/.vite/deps/rescript-relay_src_RescriptRelay_mjs.js?v=0d390227:353:21
Main@http://localhost:9999/src/Main.mjs:11:21

Document how to handle routes that require authentication or routes that find they don't have access to a resource after trying to fetch this from a remote resource (e.g. anonymous access is allowed in general but for this resource authentication is needed).

Good idea to build and install the package in the future example project, to ensure everything keeps working between each commit.

Code action that adds a full new route file a position, and opens that file.

No point in having it, can just call the CLI rescript-relay-router.

We currently have type safe route creators. They're fantastic for generating links in a type safe way. However, they're not great for generating new URLs from existing ones.

Imagine I'm in a backoffice on /organization/123/members?search=Name&order=asc&orderBy=created. Switching any of the query params here is easy thanks to our Route.useQueryParams hook which let us spread the old params before setting new params, allowing us to just update the query params we want while retaining the rest. This hook also performs an actual update, it doesn't give me back a URL I can stick in a link.

Imagine I want to switch out what organization I'm on. I'll need to run Route.makeLink again and re-insert all query params by hand to the value they currently are. Depending on where those query params are controlled, it might be easy or hard to even access them where I'm creating my link.

Proposed API:

let {makeLink} = Route.useDynamicLink()

<RelayRouter.Link to_={makeLink(~params=currentParams => {...currentParams, organizationId: Some(newOrgId)}, ())

The difference to the current method exposed by Route.useQueryParams is that this would also allow setting any path params, and generate a full new link from that. If we leave it at just generating the actual URL, one could delegate all of the "tough" decisions around "should I update the URL or not?" to the user by letting them choose - stick it in a link, use the programmatic push/replace API (with potential shallow support in the future), etc.

This could be generated in addition to the current makeLink, which still would be the most used API.

The generated route declarations currently pull in makePrepareProps and makeRouteKey from the generated route files. But, we know how to generate both of these, so we could inline them into the generated code directly, instead of risking bundle bloat because we're pulling in just a small part of a larger generated file.

Since we use jsonc under the hood, we're allowed to have comments in our route definition files. See if we can pick up these comments when they're on the lines above a route definition, so we can reprint them to appear in ReScript's hover.

We've chatted about it for a bit but since Discord is down I figured I'd create a GitHub issue instead 🙃

I'd like to move the repository closer to a monorepo structure so we no longer need the buildPackage.sh script. This would also make it easier to figure out what goes in "Application" and what goes in "Router" (and supporting packages).

I'm thinking of something like this:

.vscode
examples/
  cloudflare-workers/
  express/
packages/
  router/
    test/
  router-vite-plugin/
    test/
  router-cli/
    test/

The existing src/ would be moved into express/ and I'd like to add a cloudflare worker example right after using miniflare to ensure that the router itself does not depend on a specific server.

The idea is that when changes to any of the packages are made the example application tests are also run to serve as integration tests.

Open Debate

The proposed structure and package names (I won't change the name in the package.json) assumes a few things:

1. The names are chosen with router- prefix so that we have the option to merge this into a single rescript-relay monorepo. With the GraphQL client and router. However, the WIP.md mentions that the router should be relay independent. I think this also means that some of the CLI/VSCode plugin/Streaming code should be moved from Relay to the router (not sure which code though)?
2. The plugin and CLI are split as separate packages. I think at least having the Vite plugin separately makes sense so to provide room for optional alternative build-systems in the future? This would possibly make installation slightly more difficult for users (since they have to install rescript-relay-router rescript-relay-router-vite-plugin). I'm not sure about the CLI. (we could of course still depend on any of the sub-packages from router itself, but at least for adding new plugins and not installing all of them that'd require a major version).
3. The application itself currently relies on https://github.com/zth/graphql-client-example-server/ which always trips me up when starting the application because it's not included. This makes onboarding new contributors more challenging. I think there's a few options here:

• 3a. Move the graphql server into this repository and automatically start it when starting any of the examples (this also allows us to use it as tests). This may not be an option if you want to be able to keep using it for other projects.
• 3b. Have a dependency on the package in the examples and automatically start it when starting the examples (we would not have control over the data in the server, which makes using it for tests more difficult. To counter this the graphql package could be made more flexible but that'd increase the amount of work we have to do)
• 3c. Keep the graphql package as-is but find a different solution (e.g. throw something together using a different GraphQL package that makes it easy to dynamically specify schema and data in tests).

Open Questions

The repository contains some files for a more full-fledge application that I don't think are needed for development of the router or for the examples. Do those have a purpose or are they historic artifacts that we could remove?

Specifically:

1. The storybook integration looks unnecessary, can it, together with the stories, be removed?
2. Can we remove postcss and tailwind? Alternatively should be move those in the simple express example or make a separate example?

Find some sort of workflow for this. Can the editor tooling handle this somehow? Maybe a rename provider that does all of this?

We've decided on an initial way to address #24 and #20 called "server handlers".

Will fill in this issue with more information/discussion later.

This needs investigation, but I suspect our mechanism for preventing prepare being called multiple times might block Relay store invalidations. I have a few ideas on how to solve this.