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

Conform a bit more to Remix and NextJS conventions #178

Open
zaguiini opened this issue Sep 21, 2024 · 1 comment
Open

Conform a bit more to Remix and NextJS conventions #178

zaguiini opened this issue Sep 21, 2024 · 1 comment

Comments

@zaguiini
Copy link

zaguiini commented Sep 21, 2024

First of all, I want to thank you for this plugin. It's a great addition! I've been using it for a while, and I'm 99% satisfied 😀 For that other 1%, I have a couple of proposed changes I can help implement, given the green light.

I wanted to create a discussion topic, but it's not enabled in this repo, so I'm creating an issue to kick off the conversation.

Make Loader and Action lowercase

This conforms to Remix. It makes sense because none of the functions are related to React, so following the React identifier convention doesn't add much. Furthermore, React Router already expects a lowercase version, so my take is that this should be a no-brainer, unless there are good reasons to stick with it?

This is the most straightforward change. Because symbols in TypeScript are case-sensitive, I think it'd be a good idea to deprecate the pascal case versions and remove them in the next major version release.

Add _loading and _error preserved paths

This one is a bit more controversial. Generouted exposes Pending and Catch and that's great because it's at a route level. However, how can we share the same behavior across route groups?

I'm also of the opinion that these route states could be separated from the route handler, from a separation of concerns perspective. That also conforms more to NextJS's loading and error states.

But I must admit that I like them at a route level too, from a colocation perspective, per route. Now, I'm a bit on the fence about retiring Pending and Catch. I would recommend renaming the expected component names to Loading and Error, though.

@oedotme
Copy link
Owner

oedotme commented Oct 14, 2024

Hey @zaguiini, thanks for the suggestions you brought to discussion!

Make Loader and Action lowercase

We initially had all exported functions capitalized, but it was breaking the HMR with Vite and caused a bad DX — so we had to go with the capitalization as a workaround. That was couple of years back #5 (comment) — I haven't looked into it recently, but I'd be happy to switch back to the lowercase version whenever we longer face development issues.

Add _loading and _error preserved paths

The group layout already supports route-based Pending and Catch components besides the Loader and Action, they wrap all children routes and that works automatically now.

Personally, I prefer route level declaration for the loading and error components, as we also have the loader and action. Would feel scattered IMO to split those into different files. More importantly it will complicate the process for detecting the routes in general, and specifically lazy-loading the routes.

Lastly, I encountered Pending at the TanStack Router (React Location formally) API, and TBH I still prefer it as it's distinct from Loader — as they are not always related (depending on the router). Also, if I recall correctly, Error was used initially before Catch, but it caused conflicts with the built-in Error type and you'd have issues if you use both at the same file. And of course it was easier to write and choose over ErrorBoundary for example ;)

Hope I covered all, please let me know what you think.

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

No branches or pull requests

2 participants