Refactor and convert project to a Maven multimodule project and sync with Spring Petclinic Rest project #43
Conversation
firasrg
force-pushed
the
refactor/9-convert-project-to-a-react-admin-app-and-sync-with-spring-petclinic-rest
branch
2 times, most recently
from
e543eb1 to
3e27365
Compare
Hi @firasrg could you please reference or create in the |
|
Hi @arey ! Ive mentioned one of them in README file, the issue ive created by myself :
There is another issue i didn't mention, and it's about updating pet of a specific owner. |
|
Hi @firasrg the issue spring-petclinic/spring-petclinic-rest#145 has been fixed |
|
Hello @arey ! Good news, so I'm going to update the spring-petclinic-rest submodule and check related scenarios. As for the conflicts, as I'm changing the full repository into something else 100% uncompatible with the original, im going to accept my changes only |
firasrg
force-pushed
the
refactor/9-convert-project-to-a-react-admin-app-and-sync-with-spring-petclinic-rest
branch
2 times, most recently
from
d8255d8 to
3f999da
Compare
|
Hello again @arey ! ive made some new updates, would you like to take a look please? It's worth noting that it's crucial for repository owners or maintainers understand the whole approach applied. I'd invite @nilshartmann to join. |
|
Hi @firasrg the github actions CI failed. Thus we focus on the I don't have any React skills so I can't really give you any feedback. Thus I let @michaelisvy and @nilshartmann have a look. |
|
Hi @firasrg , thanks for your effort and "sorry" for beeing so late with commenting. I like the git submodule idea to include the "original" spring-petclinic-rest example. Actually I'm not sure if using react-admin as base is a good approach. While I think react-admin has its benefits, I'm not sure, how many people would actually use it in a "real" application. I think using a stack with React Router and TanStack Query would be more mainstrean and more realistic for real applications nowadays. To answer this question, we might discuss what the overall idea of this project is (a question that also belongs to "my" original version of the "React Petclinic"): from a Spring perspective the only thing relevant is the (REST) API. Consuming this api is not related to spring, but specific to React, Angular, whatever framework. My idea was to provide an example for people that are more or less new to frontend developement with React (and also to build a "competitor" to the angular example 😉) providing them with best practices, a base for getting started with own projects etc. That's the reason why I think, React Router/TS Query would be a better fit for this project. Using React Router/TS Query we could also show some newer React features like Suspense, advanced caching and pre-fetching with TS Query, and how to generate TS/zod types from an OpenAPI description and more... And someday maybe even some serverside features using React Router v7 (ex Remix). Beside of that I wonder if it would make sense to add spring security to the example, for example with Spring Auth Server or Keycloak. That would make the example even more realistic and could help people understand how to implement such a cruictial feature. What do you think? (btw. in a new code base I wouldn't use TypeScript |
|
Thank you for your feedback! I appreciate the suggestion to use only and natively React Router and TanStack query. While I agree that both are sufficient tools for handling the app, I chose React Admin for this rework for several important reasons:
👉 React Admin offers the same rapid development experience for the frontend as Spring Boot does for the backend—balancing speed and flexibility. I believe this approach will keep the project scalable and maintainable, while still allowing for custom configurations when necessary. TBH I was expecting you to be happy for using this framework @nilshartmann. If you still feel not ok with this approach even after reading this message, then just tell me so I cancel this PR. |
@arey I don't see a reason to replace maven build by npm. Knowing that the npm build is managed within maven. I wanted to keep the project with Maven as it's the original build tool used and as there is another Maven subproject (which is the git submodule). Ive checked the error, it seems that the problem is not related to the client module, but to the submodule: Error: Child module /home/runner/work/spring-petclinic-reactjs/spring-petclinic-reactjs/spring-petclinic-rest/pom.xml of /home/runner/work/spring-petclinic-reactjs/spring-petclinic-reactjs/pom.xml does not exist |
|
@firasrg I'm not very familiar with git sub-modules. Like @nilshartmann I think it's a good idea. Could you split this Pull Request in order to merge the submodule part? I'm ok to keep the maven build. I wasn't aware that the java backend would still need to be built. |
|
@firasrg maybe you need to explicitly need to tell the github checkout action to also checkout the submodule? https://stackoverflow.com/a/65091631/6134498 |
firasrg
force-pushed
the
refactor/9-convert-project-to-a-react-admin-app-and-sync-with-spring-petclinic-rest
branch
from
f1f4352 to
ae1dba0
Compare
@nilshartmann done ! It seems that such change requires an apporval from repository maintainers. As for the work done, I'd like t know your final decision about using React Admin please |
|
@arey I just noticed that build has failed, it seeems that the config i added isn't useful, so im going to do a second update now, it could work. |
firasrg
force-pushed
the
refactor/9-convert-project-to-a-react-admin-app-and-sync-with-spring-petclinic-rest
branch
from
ae1dba0 to
01e4471
Compare
|
@firasrg for my point of view, the aim of this demo application is to explain to junior React developers how to build a React application using best practices and without a structuring overlay like React Admin. |
|
@arey thank you for your feedback. I think that Spring PetClinic is also meant to help various levels of developers, especially those coming from other tech stacks. Additionally, when I read the introduction of this project, I find that it's designed to show good practices:
What's the definition of "simple but powerful" here please ? As I mentioned before, React Admin doesn't add complexity, it streamlines the React dev by reducing redundancy, time-consumming configurations and boilerplate. It's a well-organized opiniated framework that applies the principal Convention over Configuration. It's doing same as what Spring Boot does for the backend, serving as an entry point for juniors (or newcomers) to Spring ecosystem. So, why we apply this practice only in backend ?
It's important to consider that the React ecosystem has significantly evolved these recent years. It's no longer just about creating some components and see magic UI interactions. Today, React involves multiple tools, typed languages, high practices, and new syntax, which can make React dev more complex without some bootstrap. React Admin also offer a flexible and extensible solution, simplifying maintenance, and keeping the project easy to read, scalable, and open to all levels |
I tend to agree here. While React Admin for sure is a valid option when building React applications it's far from being mainstream (have a look at the npm download stats). Again: that does not say anything about the quality or the pros and cons of RA. On the other hand, libs like React Router or TanStack Query are quite popular an can be considered something like a "de-facto" standard for new React applications (they are even mentioned in the official React docs). To some extend I think this is also true for react-hook-form and zod. And I think as an example application here, we should follow a mainstream path being as unopionated as possible. Having said that, I really appreciate your efforts here and wonder if @arey idea ("This could be a dedicated repository (or branch)." would be an option. What do you think? |
|
@nilshartmann What if I tell you that React docs itself recommends to use a React framework for a new project ?
FWIW, I've worked with ReactJS for years and have been in touch with developers from many levels, just to say that I know what I'm doing. However, if you still don't agree on using a framework like this one after all the details and questions I've given you, I'll have no choice but to refactor the client app, removing the framework and re-founding it to a traditional React app |
|
@firasrg The term "framework" there refers to a framework like Next.js, Remix or Gatsby. In other contexts those frameworks are also called "meta frameworks" because they're providing build tools, bundling, serverside routing and rendering and more. (I think somehow very carefully comparable in the way we call Spring Boot a "framework"). And again: this not about React Admin itself. I just want to raise the general question if we want to go with a more "mainstream" or "opinionated" way here. |
RA is very similar to Gatsby and Remix.
It's the defacto today.
And no problem, as I said previously, im going to have to refactor stuff, uninstall RA and try to release a basic React app with some known tools including TQuery and ReactRouter, is that ok ? @nilshartmann |
Sure! Please let me know if I can help. |
Comments: 1. empowering code style with eslint-config-universe and prettier; 2. use modern routing with react-router-dom; 3. remove unused files; 4. Fix layouts, icons and styles
firasrg
force-pushed
the
refactor/9-convert-project-to-a-react-admin-app-and-sync-with-spring-petclinic-rest
branch
from
2bbefa9 to
b065222
Compare
|
Hello @arey and @nilshartmann and everyone 😁! I've updated the PR with significant changes based on your feedback to align with the project's goal of showcasing best practices with minimum React tools. Summary :
Could you please review the updates and let me know if there’s anything else to address? I’d appreciate your feedback to ensure this PR meets the project’s goals. Looking forward to merging this to enhance the Spring PetClinic ReactJS project and become one of its contributors |
|
Hello guys, any updates please? @nilshartmann @arey |
|
|
||
| The choice of using **Spring PetClinic Rest** as Git Submodule ensures that the backend can be updated independently without requiring code duplication. It also keeps the project stable by locking a specific version whenever there are breaking changes in the **Spring PetClinic Rest** subproject. | ||
|
|
||
| NOTE ⚠️: _Some CRUD functionalities in the client are not fully implemented yet due to unresolved issues in the **Spring PetClinic Rest** repository ((like fetching Pets of a specific Owner. Currently, there is an [opened issue](https://github.com/spring-petclinic/spring-petclinic-rest/issues/145) about this), these will be addressed as soon as the backend fixes are made (by community), ensuring that the client-side work can proceed smoothly._ |
There was a problem hiding this comment.
question: the issue 145 has been fixed. Did you recently test all the CRUD features?
There was a problem hiding this comment.
No, I think that I've got to update the rest api submodule
|
Hi @firasrg, thank you for taking into account all the changes. |
|
Hi! Sorry for coming back very late, but I'm currently very busy. I add some quick notes (haven't looked deeper yet) to the PR (haven't looked deeper yet) |
| total?: number; | ||
| } | ||
|
|
||
| class ApiService { |
There was a problem hiding this comment.
Why a class here? Not sure, but couldn't you just export a bunch of methods for the other modules?
There was a problem hiding this comment.
Primarily for encapsulation and organization, it groups all related API fetching and manipulation methods within a single, cohesive unit, which I believe enhances readability and maintainability as the application grows. Additionnally, it provides a clear structure and keeps the door open for advanced patterns like DI or extension
There was a problem hiding this comment.
True, but I don't think it needs to be a class (while beeing a common pattern, it seems unconventional in JavaScript/React apps). I'd just export the methods.
| import { usePetTypes, usePet, useCreatePet, useUpdatePet } from "@hooks/usePets"; | ||
| import { formatPersonFullName } from "../../utils"; | ||
|
|
||
| const yupSchema = yup |
There was a problem hiding this comment.
I wonder if zod would be a more state-of-the-art option than yup, esp. it also can infer the typescript types.
There was a problem hiding this comment.
That's an excellent point, IMO it's a more state-of-the-art option, particularly for its superior TS inference capabilities, which would indeed streamline our type definitions and validation
There was a problem hiding this comment.
If you're interessted, I could migrate this branch to zod.
| const createOwner = useCreateOwner(); | ||
| const updateOwner = useUpdateOwner(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I would move the loading the owner out of the OwnerForm.
That way there is no need for useEffect anymore (just pass the owner as props to OwnerForm). Also you than might need to have a dependecy to the router here.
There was a problem hiding this comment.
In App.tsx, OwnerForm is defined as a route for both OWNERS_ADD_NEW and EDIT_OWNER paths. Using useOwner and useParams directly in OwnerForm keeps it self-contained, handling its own data fetching and routing logic, which aligns with its role as a route component. This avoids prop drilling from the App component and leverages the useOwner hook’s encapsulation. The useEffect syncs the form with fetched data in edit mode, which is necessary for the route’s functionality.
Given OwnerForm’s role as a route component, keeping data fetching internal simplifies the codebase and enhances reusability. Could you clarify if there’s a specific project convention that favors moving data fetching to the parent App.tsx ? I think this would lead to tight coupling and unnecessary charge in the App component.
There was a problem hiding this comment.
I would prefer separating loading the data because the ownerForm then can edit an owner no matter where it comes from. And then useEffect is not neccessary anymore, which is a benefit, because useEffect is hard to understand and it easy to make something wrong here. Also handling loading and error state (for getting the owner) can then be removed from the OwnerForm.
Overall I think by splitting the code the overall code get's easier to understand.
There was a problem hiding this comment.
Sorry I don't get what you mean here, the component OwnerForm is just under App, do you suggest to make a HOC for its api logic that renders the Form?
There was a problem hiding this comment.
I only would move the loading feature out of the component. So that it does get it's data from outside (or no data at all for new owners)
| @@ -0,0 +1,7 @@ | |||
| export enum EOwnerForm { | |||
There was a problem hiding this comment.
imho typescript enums are effectively "deprecated". If there is no strict reason I would go for const objects (https://www.typescriptlang.org/docs/handbook/enums.html#objects-vs-enums). Using enums typescript tooling has to generate runtime code, while with objects it does not have to (makes it easier for tooling)
There was a problem hiding this comment.
Sorry I don't see any official msg about its enums deprecation, but I see how they can be replaced with Objects
There was a problem hiding this comment.
I'm sorry, that why I wrote "effectively" deprecated - bad wording, my fault. I meant it seems its no longer best practice to use them. Actually there is a compiler flag to avoid using these TS features: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-8.html#the---erasablesyntaxonly-option
|
|
||
| export const PET_TYPES_QUERY_KEY = "petTypes"; | ||
|
|
||
| export function usePetTypes() { |
There was a problem hiding this comment.
Regarding the hooks for data fetching: I think in "modern" TanStack Query apps, you would not use hooks for reusable / centralized query definitions, but queryOptions
| } | ||
|
|
||
| export default function NavigationBar() { | ||
| const [activePage, setActivePage] = useState<ENavBar>(ENavBar.HOME); |
There was a problem hiding this comment.
what is activePage for? Couldn't you infer that from the router?
| ...options | ||
| }; | ||
|
|
||
| const response = await fetch(url, config); |
There was a problem hiding this comment.
Maybe we could some validation here somewhen later
There was a problem hiding this comment.
Yes, exactly! I have a common fetchFromApi function that validates responses received from the backend. If you're interessted, I can share it with you. (works with zod)
| import { formatPersonFullName } from "../utils"; | ||
|
|
||
| export default function VeterinariansPage() { | ||
| const { data, isLoading, error, refetch } = useVeterinarians(); |
There was a problem hiding this comment.
we could consider using suspense hooks for data loading
There was a problem hiding this comment.
Sorry I don't get this, can you give more details or share an example please ?
| } | ||
|
|
||
| const onSubmit: SubmitHandler<PetFormSchema> = async (data, e) => { | ||
| e?.preventDefault(); |
There was a problem hiding this comment.
Is this neccessary with react hook form?
| const yupSchema = yup | ||
| .object() | ||
| .shape({ | ||
| [EPetForm.NAME]: yup.string().required(REQUIRED_INPUT), |
There was a problem hiding this comment.
Not sure if it is neccessary to use constants for field names here, as the react hook form api already makes sure that only valid field names are used. If you use zod you also have one "single point of truth" for both validation logic and typescript types, reducing the need of constants for field names even more.
There was a problem hiding this comment.
True, but constants help to well remember the fields and to avoid having to rewrite their names literally, it also gives better readability when it's in uppercase
There was a problem hiding this comment.
- remember the fields: isn't typescript support not good enough? I mean you get code completion across (almose) the whole react hook form api
- better readability when it's in uppercase: i don't like my code screaming at me, it's enough when I scream at the code (in case it's not working) 😉
There was a problem hiding this comment.
I didn't say that TS isn't enough, and uppercase doesn't mean screaming, but ok
|
Hi @firasrg ! If you're interessted, I could help with this branch, I have some time the upcoming days. |
|
@nilshartmann I appreciate your willingness to help, and I wouldn't mind. However, I think I can handle most of the reviews, except for a few I haven't understand yet. There's some more work to do, which is updating the REST API module and re-checking the CRUDs. If you'd like to help, would you like to continue alone, or would you like to work on the same branch with me or do you have your own? |
4 participants
This PR introduces a major rework of the Spring PetClinic ReactJS version, converting the project into a multimodule Maven setup, with two distinct subprojects: spring-petclinic-reactjs-client and spring-petclinic-rest. The spring-petclinic-rest subproject is integrated as a Git submodule, allowing for smoother backend updates without duplicating code, and maintaining stability by locking specific versions when needed.
My main focus is on the ReactJS client, which has been significantly enhanced with modern technologies such as React v18, React Admin, Typescript, and Vite.js. While some CRUD functionalities are not yet fully implemented due to pending issues in the spring-petclinic-rest repository, this structure lays the groundwork for a scalable and maintainable application. Once the REST project issues are resolved, the corresponding client-side updates will be added.
This PR should be related to #26.
Also, I'd like to thank @michaelisvy for their early collaboration. Im open to discuss about any aspect of this new form. I hope you'll like it.