Solid adapter #25
Solid adapter #25
Conversation
|
This is awesome - thank you for working on this!! I will try to take a look at this in the coming week or so as I get back into the swing of things post-holiday 👍 |
|
Great to hear that, hope you had fun on your holidays. You can go through the pr in your suitable time. Anyways, I think I could make few more improvements to pr, mostly combining all files in components folder to one file and others. I will make those changes after your feedback |
|
This looks great! The app looks right to me - I haven't written any SolidJS before so I can't speak much to the specifics but overall the approach and touchpoints with the
Yeah I would ignore relative routing and any I'll go through and leave any comments, but generally I'd say feel free to go through and do any cleanup you'd like. Once that's done we can get it merged to |
| @@ -0,0 +1,34 @@ | |||
| ## Usage | |||
There was a problem hiding this comment.
Let's update this with something like what Vue/Svelte has that shows an example of how to use this in a new Solid app
| @@ -8,7 +8,7 @@ export interface TaskItemProps { | |||
|
|
|||
| export default function TaskItem({ task }: TaskItemProps) { | |||
| let fetcher = useFetcher(); | |||
| let isDeleting = fetcher.formData != null; | |||
| let isDeleting = fetcher.formData != null; | |||
There was a problem hiding this comment.
I think changes to this file are unintended :)
| "typescript": "^4.9.4", | ||
| "vite": "^4.0.3", |
There was a problem hiding this comment.
Out of curiosity, do we need these updated versions for Solid? Or would the versions installed in the repo root package.json suffice? If these are required for Solid then no worries and eventually I'll get the root versions updated and we can drop these from the dependencies.
| "vite-plugin-solid": "^2.5.0" | ||
| }, | ||
| "dependencies": { | ||
| "@remix-run/router": "0.2.0-pre.10", |
There was a problem hiding this comment.
This has been released as stable since the repo was created. I wouldn't touch anything in this initial PR - but maybe in a follow up we can get it onto the latest (1.2.1, soon to be 1.3.0)
| @@ -0,0 +1,69 @@ | |||
| import type { Component } from "solid-js"; | |||
There was a problem hiding this comment.
Small typo in the folder name her - reference-app
| const fetcher1 = router.getFetcher("1"); | ||
| const fetcher2 = router.getFetcher("2"); | ||
|
|
| export const useLoaderData = <Data>(): Accessor<Data> => { | ||
| return useRouteLoaderData(useRoute().id) as Accessor<Data>; | ||
| }; |
There was a problem hiding this comment.
These generics might be slightly misleading. If you return a JSON Response from your loader it'll actually automatically extract the body via res.json() so I think the types might get wrong there.
// The return value here is Response
function loader(): Response {
return json({ message: 'hello' });
}
// I think this will be typed as Accessor<Promise<Response>>
// but it's actually of type { message: string }
const data = useLoaderData<ReturnType<typeof loader>>();We left them out of the initial versions of these in react-router to make sure we eventually got them right, so I might suggest the same in the Solid version.
|
Thanks for the feedback. I will work on those over the week and update the pr |
|
Sorry, I got busy with some other work. I will try to finish this by week |
|
No rush at all! |
|
Any updates? |
This pr adds support for solid.js adapter for
@remix_run/routerpackage with all tests passing.In some cases there are differences between
react-routerandvueimplementation, for exampleLinkcomponent in vue does not supportrelative-pathbutLinkinreactdoes. I have implemented similar to vue only (i.e solidLinkalso does not supportrelative-path). Let me know If i need change that too.Let me know if anything more needs to be done