Skip to content
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

Add post_processor to customize the output of each route #306

Closed
wants to merge 2 commits into from

Conversation

vovanmozg
Copy link

Sometimes we need to customize the output of each individual route.

I was faced with the task of typing routes when used with react. And I couldn't use dynamic generated routes with toString(). This method returns string type for any helper. It would be great for me if it was possible to export patterns as literals

@bogdan
Copy link
Collaborator

bogdan commented May 16, 2023

Can you give some live examples of how post_processor would look like in your app?

Your solution looks pretty hacky:

  • I don't see how it would solve any other problem than what you are solving
  • It is not universal in terms of module_type
  • It may need additional parameters not just route like absolute.
  • It increases routes file size by probably around 30%.

I actually think that exposing route spec via something like Routes.inbox_path.spec(): RouteTree is a good idea. It seems only takes 3 lines of code to implement and doesn't cost much in terms of support.

@vovanmozg
Copy link
Author

vovanmozg commented May 16, 2023

I regret that I did not motivate PR well enough.

Consider the following example.

I have React Router routes description in routes.ts file:

import React from "react";
import ReactDOM from "react-dom/client";
import { createBrowserRouter, RouterProvider } from "react-router-dom";

const router = createBrowserRouter([
  {
    path: "/settings",
    element: <div>...</div>
  },
  {
    path: "/profile",
    element: <div>...</div>
  },
]);

ReactDOM.createRoot(document.getElementById("root")).render(
  <React.StrictMode>
    <RouterProvider router={router} />
  </React.StrictMode>
);

In this case type of path property is union of two literals "/settings" | "/profile".

I can use .toString() to define path:

const router = createBrowserRouter([
  {
    path: Routes.settings_path.toString().replace(".:format", ""),
    element: <div>...</div>
  }
]);

But in this case the type of path will become just a string. But I know for sure that the path is only limited to a certain subset. And I want to use this knowledge for type checking

@vovanmozg vovanmozg closed this May 16, 2023
@vovanmozg
Copy link
Author

Yes, the solution looks pretty hacky. I don't like that the post_processor has to deal with an already formed string. Perhaps it should receive not a string, but an array of components [comment, name, body].

@vovanmozg vovanmozg reopened this May 16, 2023
@bogdan
Copy link
Collaborator

bogdan commented May 16, 2023

I am not sure how your solution is going to work.
At first, settings_path() and profile_path() will always return a string because it is dynamically constructed.
So your only option to always use /settings and /profile when you would want to actually use the path because the follwoing will result in an error:

const settings_path = (): string => "/settings";
const path: "/settings" | "/profile" = settings_path();

So, even if you can dynamically define a router using js-routes helpers, you would have to hardcode it later. So, what is the point of avoiding hardcode with js-routes in the first place?

Other thing is route parameters like /posts/:id. I don't see any way to ensure certain variable can only be assigned a result of post_path(id) but not message_path(id). Do you plan to support parametrized routes in the first place?

@vovanmozg
Copy link
Author

Lets look at test one more time

Proc.new do |content, route|
  if route
    # This may be a more complex replacement.
    pattern = route.path.spec.to_s.gsub('(.:format)', '').gsub('.:format', '')
    content + %Q[,\n#{route.name}_pattern: '#{pattern}']
  else
    content
  end
end

https://github.com/railsware/js-routes/pull/306/files#diff-544bb65018327fd52b3c481ebef39d767ee5e876a08a89fe2037901b82f3f91d

It generates the code:

/**  
* Generates rails route to  
* /inboxes/:inbox_id/messages(.:format)  
* @param {any} inbox_id  
* @param {object | undefined} options  
* @returns {string} route path  
*/  
inbox_messages_path: __jsr.r({"inbox_id":{"r":true},"format":{}}, [2,[7,"/"],[2,[6,"inboxes"],[2,[7,"/"],[2,[3,"inbox_id"],[2,[7,"/"],[2,[6,"messages"],[1,[2,[8,"."],[3,"format"]]]]]]]]]),  
inbox_messages_pattern: '/inboxes/:inbox_id/messages',

Then I can import and use inbox_messages_pattern to define React Router item:

...
import { inbox_messages_pattern } from 'routes';
...
const router = createBrowserRouter([
  {
    path: inbox_messages_pattern,
    element: <div>...</div>
  }
]);

And I don't have to hardcode literal (/inboxes/:inbox_id/messages) this place. It is way to avoid routes duplication.

@bogdan
Copy link
Collaborator

bogdan commented May 17, 2023

I think the best way accomplish that is the following:

  1. Disable format option for routes. I believe most apps are no longer using format at all.
  2. Use inbox_messages_path.toString()
scope format: false do
  resources :inboxes do
    resources :messages
  end
  ...
end

Any problems with this approach?

@vovanmozg
Copy link
Author

In this case, the type of path will be a string. Ideally, I would like to limit the type to only possible values (existing routes), but this option is quite acceptable.

This was the first option I considered before doing PR:
inbox_messages_path.toString()

And I'll probably stop there for now. For current tasks, this will be enough. While I ask you not to close this PR, suddenly new ideas will arise :)

@bogdan
Copy link
Collaborator

bogdan commented May 17, 2023

So what we can do. First, rename toString() to spec() because of some limitation over the return type of toString() (honestly, naming it toString() always was a bad idea).

type RouteHelperExtras<T extends string = string> = {
  ....
  spec(): T;
}

Make sure we generate RouteHelperExtras<"/settings"> with appropriate spec here: https://github.com/railsware/js-routes/blob/master/lib/js_routes/route.rb#L43

The only problem of such approach: it will not strip optional parameters like :format automatically.

@bogdan
Copy link
Collaborator

bogdan commented Jul 5, 2023

I decided that it is too much of an overhead and isn't applicable to all use cases to accept it to the core. So closing this one until a better solution will be found.

@bogdan bogdan closed this Jul 5, 2023
@vovanmozg
Copy link
Author

I agree with you. The idea was underdeveloped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants