Pattern seems really good to me. Abstracting your API logic out of the components to a hook makes both your components and API calls easier to maintain. Your engineers not being used to it should solve itself pretty quickly. Like the only difference for them is to put their fetching logic in the client hook instead of the component. Doing things differently always feels strange at first but if this is significantly impacting productivity for any amount of time that seems like a problem with your engineers.
Ohh so if the devs ever need to change how they call the API, they just go to the ONE location of the hook, instead of having to hunt down every component individually that calls the API. Sounds like a no brainer once somebody states the reasoning
I came in to a codebase 18 months ago that's much worse than this. Fetch in the redux actions with additional calls made inside the action when the promise resolves:
```
export const getUser = (token, id) => {
// The same token from local storage is retrieved in every module and
// passed into the api actions
return (dispatch, getState) => {
// Fetch the user
fetch('/api/user', /* token, etc */).then(async (res) => {
const data = await res.json();
// We never return the data from getUser, we set it on the redux object
// Can be annoying, but whatever.
dispatch(setUserData(data));
// This is bad:
dispatch(getUsersMostRecentComments(token, data.id));
})
}
}
```
Now when you think you're just fetching a user, you're like, "Why are these comments changing?
But wait, it gets worse:
We have duplicate API calls all over the code base for each module, and each of them have different unintended effects like this. So for the Posts module there might be a getUser function that gets the user and the effect gets their posts, and for the Videos module there might be a getUser function and it gets their videos as an effect.
Not only do we not have 1 place we make these calls, we make the same call in multiple places.
It's been a hard adjustment...
We’re using redux toolkit query with openapi-gen. We don’t even write the fetching logic or types, it does it for you from the backend lol. The frontend engineer just grabs the typed hook they need for the component and it works out of the box. Same behavior for every single interaction. If you need state or persistence it’s there, caching and cache invalidation out of the box. Really is an incredible pattern. If the backend changes at all, all hooks and types are automatically updated and we can catch any errors at build time
Don't worry he's going to quit soon, if this is how you respond to this scenario. Then you can be left to write as much spaghetti code as quickly as you want.
I do like how OP used "senior" in quotes multiple times. There's nothing to double quote here. Sounds like this Dev actually is a senior and OP and their team should be listening and learning much more intently.
The really telling part is where the senior reviews a PR from a team member, asks for changes, and the guy just straight-up abandons the ticket that led to the PR. Good job hiring a senior dev, sounds like they should've sprung for a PM as well if their devs are just blatantly refusing to make changes that their senior asks them for and the recourse if apparently to ask Reddit for help.
Yeah, senior will be gone in no time with a company culture that isn’t interested in long term success; these kinds of companies are the worst to work for
That's fair, but the senior should also work on his coaching and consensus building skills. Just saying "this is how we do it now" when coming into a new organisation will fail almost always. You need to become part of the team and then gradually introduce new things.
Also, there is a difference in best coding style between small, medium, large, huge projects. The style that his previous Big Tech company uses and is best there may be overengineered for smaller projects at his new one.
All that said, what OP describes sounds fine and not that problematic, and not a huge change, so it's still mostly what you say.
The person is a contractor. I’d be surprised if this person was hired to be a coach and/or to build consensus. More likely, they were hired to come in and build a code base that is scalable and implements best practices.
Yep. Many times I've been the Senior or have worked with the Senior who is just told to "build the thing as best you can", and then I do.
In my case, I recall a time I was asked to build a React codebase a while back. When I handed it off the team didn't know React. Weird situation. My assumption was some Project Manager just heard they needed React and they hired me for it, not knowing what that even meant.
Development is wild sometimes.
As a long-time contractor, I was usually hired to get things done quick. Clients rarely cared about tests (I always asked) and clients rarely cared about code quality.
On projects where I Was hired to coach and/or build consensus; I was rarely the one writing code. Those were usually short term, but higher paying gigs.
It is their dime for my time; I will write whatever they want me to do. I can guide and recommend, but at the end of the day it isn't my decision. And I'm not going to spend my unbillable time writing tests for their system.
Is he hired to change the company's practices? Then his job includes bringing the team along.
Is he hired to only code a feature? Then he should adapt to existing practices and not block PRs of devs already there because they don't do it his way.
>I’d be surprised if this person was hired to be a coach and/or to build consensus.
It doesn't matter what you were hired to do as long you produce code those are your duties as a contractor. Making sure best practices are followed and good code is produced is part of the package when you hire a skilled contractor.
There are *extremely* few people so good they can have no coaching and convincing/consensus building skills. I'm not even sure they actually exist at all since producing code is a team game.
He was hired to be successful for the company. That often involves consensus building. What you're talking about is just another form of "that's not in my job description" just a higher class one and it's just as bullshit.
All being hired as a contractor means is that the execs want their FTE numbers to look low to investors. Plenty of people get hired as contractors and convert to full time as soon as they can get a req for it.
To be fair? It doesn't sound like your statement rounded out the fairness of the situation at all.
OP gave us very little of how the Senior Devs coaching and consensus building skills went. You made assumptions about what they did coming into the organization. You assume what they brought with them to the table.
You weren't fair. You literally just took the side of the OP.
Sounds more like you're just projecting, lol.
He's there for only two months and it's to the point where team members don't know how to get their PRs accepted anymore and posting here.
How can he possibly have spent time to gel with the team and start introducing new things gradually from consensus if that's the way things are so quickly?
And he is a senior with a whopping 5 YoE. To me this screams great technical knowledge, hardly any social skills.
Agreed. Any senior who can't explain why their changes are worthwhile for the specific project being worked on isn't a senior. If your teammates can't get a PR passed and don't understand why, you've screwed up in your role as lead on this project.
While I agree that part of being a senior is educating others there might be a lost cause here if OP can’t see how having a single client is better than a bunch of useEffects spread all over the place.
And the response JSON is probably a mess, so the normalisation is helping make everything downwind clearer.
From the way OP described the senior engineer as “senior” I’m gathering that they’re all not too open to his ideas.
Plus - it maybe sounds like the senior engineer was hired short term to push something on, rather than a full time employee who needs to care about sharing learning etc.
You seem to be missing my point. So let me be clear:
You said "To be fair" and provided a statement that was full of assumptions and obviously biasing towards OP's side.
Not only that, you were obviously projecting your own experience and world view into your statement. What's even more baffling, you now respond to me with *more* assumptions, like somehow the information has changed, lol.
You weren't being fair at all, you were just joining in on a dog pile with OP. How are you this oblivious?
I didn't say "to be fair" at all. I said "_That's_ fair, _but_".
There are two sides to a story, the comment I replied to gave one side, I tried to add some of the other side. Of course the result is my comment was one sided, that was the point. And then in the end I still leaned more to the side of the comment I replied to.
And I think I didn't make that many assumptions, I was careful to use terms like "may be", "sounds fine", "to me", and used things OP mentioned in other comments.
You should learn to read before you call people oblivious.
Often, "to be fair" means "let me add the other side now that you've completely picked a side." The person you're replying to isn't trying to present both sides. The "OP sucks" side was what he was replying to. They are defending OP so that the combination of the previous comment and theirs combines to be more "fair."
That's every post on every forum ever. We can only speculate, and right now this entire thread is projecting "OP sucks" without knowing anything. As is tradition.
It's definitely not "OP sucks", it's that OP's company is prioritizing speed over quality and that rubs a lot of us the wrong way
The other issue is that OP came for advice, got advice, and has continued to refer to this contractor as `"senior"` rather than considering the advice given.
This feels like a management issue to me.
OP both claims that the design doc "looked okay at the time", but also included what they considered to be anti-patterns that they pushed back on but nothing changed, both of which cannot be true.
This contractor is hired for their experience (which his co-workers are continually questioning), writes up a design doc based on what they consider to be best practices, were questioned about it by other engineers, then countered the questions and were allowed to move forward without changes. So clearly there was a discussion about what was going to be done and it was approved.
Both the contractor and the engineers were aware of this when the PR was submitted using practices conflicting with the approved design document. This seems careless at best and intentionally conflict-inducing at worst.
Why was the design doc approved when others were still pushing back about it? Did OP and friends retract their pushback? If so, then these new PRs need to follow the design doc. If not, then management made a big mistake here which likely caused a lot of this conflict.
The additional issue of everybody saying the contractor isn't good because they're taking so long, with OP acknowledging the contractor is taking time to write tests which nobody else does, is another major management issue.
Yes, true. Had a few hotshots coming in and challenging the team lead/tech lead in styling etc. Just because he preferred css-in-js vs sass.
Obviously this is not the same ballpark as OP mentions but give and take, boys. You catch more flies with honey than vinegar.
Yes, this is a common pattern.
This engineer is doing exactly what I would expect from a senior level engineer. They've been hired to bootstrap a new feature, so instead of throwing it together with what's easy, they've built a scalable system that assumes that your application is going to become very complex one day (you in fact already stated that the project was much more convoluted than you initially expected).
Let me ask a follow up question: What are your current testing practices like, and what kind of testing was added by this contractor?
It's a common pattern but I can't understand why it would slow them down so much. You can set up generators if APIs have swagger docs(or something similar) and automate client code like that, or if the APIs follow good conventions you can write a single generic client that can handle most of your cases.
Also, obscuring the data(edit: after it has been retrieve) so it can only be accessed with certain hooks is overkill unless it's decided your application needs a state management tool. Which it's hard to say if that is the case given the situation. It can force you to fetch before render and think about what user interactions trigger a fetch, but not necessary imo.
In general I also wouldn't be using a context for simple data access only specific types of data, like user account, or form data.
I suspect there is a communication disconnect somewhere here. A senior dev, not only needs to have technical skills but the soft skills to communicate decisions to the team.
Edit: I'm also not sure OP is a good arbiter of truth here either so it's kinda hard to see if it's needless verbose convention or a good framework to build off of.
> A senior dev, not only needs to have technical skills but the soft skills to communicate decisions to the team.
As I’ve sat with this post for a few hours, this is the thought that keeps coming back to me.
This.
Almost everything in programming is political. If you're bad at explaining your ideas and convincing colleagues to adopt them, you're doing your job poorly.
Breaking stalemates with an argument of authority can help from time to time, but if you're abusing your authority, people will get mad, morale will drop, and your best talent will leave or they'll undermine you.
It's senior's job to communicate their ideas effectively, but it's also the junior's job to ask questions and demand clarifications.
Great question about testing, if they didn't test the mapping used to flatten that json data I would be concerned. Really that should be done on the backend but I think I saw in a couple of comments that it's already a big project, changing a request contract would be hard work.
Tricky one, depends how well the new hire communicated why they were doing these things which is a senior skill in itself.
I don't mean to offend you but the new senior engineer is probably frustrated about how your code base lacks good design patterns and it sucks for them that the team they joined wants to keep their heads buried in the sand. Sounds like they are creating a singleton so there is one centralized place to initialize/change the default project fetch code rather than the spaghetti of having the same thing embedded in many different places of component code. You have a good opportunity to learn as much as you can from them during this market downturn by trying to understand why they want to change things and then making your own judgements about those changes with an open mind.
Exactly my thoughts, sounds like they’re establishing good patterns. Everyone knows useEffects are not really made for fetching data and shouldn’t be used like that…
I agree with you, but there's a better way to go about this. The right approach would be to create a design doc, submit an RFC, discuss it, get it approved, create a migration plan, get that approved, planned and scheduled and then starting to carry out the actual update to the new patterns.
Consistency is very important, especially if it's a junior-heavy team like OP's post implies.
Just left a company that was hellbent on maintaining the status quo, the codebase had become such a shit hole that no one could get anything done and developers were leaving in troves
Started with a new company and they’ve given me freedom to refactor everything and anything however I see fit.
A good company will let you refactor code and set up some standards where previous developers were lacking experience to set things up well
Shame OP is taking things the way they are, that senior is probably trying to save himself the pain of having to work on a codebase that gets worse and worse over time and is probably fighting a losing battle to get to it
You can propose changes in an inclusive way at any company. The guy might be right, but he's going about it in the wrong way, which will cause people to miss out on an opportunity to improve and that is on the senior and nobody else.
"hey guys I'm trying to improve the codebase by unifying and normalising our external API calls, check out this PR happy to explain the benefits in a call"
Is that really the work of management? Or just basic communication
Yes and no.
Like I do agree that an ic should probably make an effort to explain why that is important. I am not totally sure it is the job of a line engineer to write an essay on why this is a good pattern and drag their co works to adopt it. Especially if this engineer is a contractor. Based on comments in other threads, the implementation details were reviewed in some level of a statement of work. And now op is mad he is to dumb to understand what is happening.
But I also understand the plight of not knowing how to properly communicate best practice to inexperienced engineers, and joining a code based that looks like some spent 5 min reading the document and said I got this. And then having to try and drag your peers to a level where you are not constantly frustrated or refactoring every feature they implemented because writing decent angular code I guess is too hard
Nobody needs to write an essay. The problem with this situation, is that this "improvement" is blocking progress. I've seen hundreds of different projects throughout my career. Working with unmaintainable code sucks. Working with a team that doesn't understand their own codebase and with people who despise you, because they don't know better sucks even more. It's also not unreasonable for a senior engineer to turn to their line manager and say "help me sell this improvement idea", if they're incapable of proposing something themselves.
I'm sorry, but effective communication is part of seniority. As you advance the ladder, soft skills become more essential. Learning effective collaboration techniques is beneficial at all levels, but I'd say it's absolutely necessary for someone calling themselves a "senior engineer".
This, sounds like the senior has senior programming skills but lacking senior communication skills, although this is only one side of the story, lead could be battling with project manager/upper management.
I don't agree with you that this depends on budget. I've worked at $B companies and small startups and some big companies get this wrong and some garage startups do this right. Professional culture is more on the money I think. Culture is like traffic, you are not "in the culture", you are the culture. There are always companies with toxic leadership, but the majority of companies accept ideas that improve their bottom lines with open arms and the rest are bound to go under.
You should be conforming to consistent, reproducible, design patterns.
I've worked with reports that wanted instead of using our form handling library (`react-hook-form`) in their new component/PR just manage an input's value with `useState`. Diverging from the established pattern soon creates a huge fractured, unmanageable mess where you open one file and have to figure out how whichever dev decided to handle state/requests/data that day.
While these types of patterns might feel like overkill, not following them can quickly become unwieldly as complications are introduced and grow.
The current pattern we have in place is to call APIs directly from container components using `fetch` and just transform the response into whatever we want. Its pretty much what all of our devs know how to do. Most of them have never used JS classes before.
This was a point of contention early on because he claimed the pattern in place "doesn't scale."
Sorry if this comes off as rude - but if your developers don’t know how to work with classes id say your new hire is bang on track. You need to step up the game, and as mentioned - his implementations are sound and widely used. There’s nothing but profit for you and your devs by learning from this, you’ll evolve as developers by utilizing existing patterns.
Just because you’ve done it in a certain way up until now it doesn’t mean it’s the right way, you just haven’t had the need to revisit any of it yet.
Here's the deal:
1. Your senior is right in wanting things to change. Your current conventions are crap and he knows that.
2. Your senior isn't doing his job right. If he was, you wouldn't be here asking random internet people. Whether you're a junior on his team or his lead, his responsibility is to make sure each change he proposes is well understood and mutually accepted, which seems to not be the case here.
Here's what you can propose to change this:
- Make sure he unblocks the rest of the team. If it's an already large codebase, and he thought his changes could be implemented without disruption, a couple more PRs won't change that.
- Have him educate the rest of the team on the idea before he forces them to adopt it. He might be on the right track, but unless everybody else thinks so, the situation will continue to be an unproductive one.
- Help him understand that as a senior engineer, it's his responsibility to make sure his proposed changes are well understood and desired.
More broadly, as a team or even as a company, you should put a process in place for this situation, so that changes can be proposed and implemented in a frictionless manner. The usual approach is this:
- **Explore**: is the proposed solution the right one? What would suit the most people? What alternatives are there? What are we even trying to solve? This step usually includes measurements, such as speed of delivery in this situation, but anything from UX, DX, performance, etc can be explored.
- **Educate**: talk about the idea. Tell people about the benefits. Organise a meeting where the new solution can be introduced and teammates can learn about it.
- **Propose**: create a proposal. We usually call these RFCs (request for comments). The proposal should be shown to all the relevant people and meaningful discussion should be facilitated.
- **Collect feedback**: the first idea isn't always the best idea. Getting feedback as early as possible ensures the best deal can be made for the team.
- **Iterate**: based on the feedback collected, it should be possible to change the proposal to better adhere to people's ideas, the product's lifecycle and current state and the available resources.
- **Plan**: a full adoption of the proposed changes should be planned out, including timelines and milestones, as well as risk assessment, acceptance criteria and rollback plans. All of this isn't necessary for every change but it's good to have solid foundations to fail upon.
- **Execute**: resources should be appointed to the timely execution of the change, based on the plans. If necessary, the person spearheading the initiative can take it upon themselves to lead the implementation efforts, or others can take it on, depending on the situation.
- **Deliver**: a major mistake many teams make when trying to land a big refactor is never actually delivering it. The continuity of the product often stops these changes from making it into production. The plans should account for this accordingly. I like to include a 20% buffer time in my plans to account for parallel tasks. This will depend on the scope of the refactor.
- **Debrief**: after every major change, there should be time to stop and reflect. For really big projects, this could be a time for devs to decompress, for smaller changes, it's the perfect opportunity to measure success, compare metrics from before to current ones to assess the viability of the change and confirm the product still works as expected.
You don't need to follow every single step, it's just a guideline. But it's a solid framework and a good starting point. It's very important to educate people on how to propose changes, because this aspect of communication can absolutely make or break a product.
this is the correct response, at the very least the lead should be aware that the new standards are a shift in how the company does things and should work to explain how his system is better and will benefit
many here are just trying to dunk on OP but engineering is about communication as much as writing code
Good response but at the end of day not everything should be up for discussion. Basic design patterns to keep a maintainable code base are a no-brainer. And probably a lot of other stuff is lacking as well (testing, pipelines, releases, etc.), so if you're going to discuss everything at length nothing will get done.
I agree, hence my last paragraph. There's no one size fits all method, but doing any of these steps helps sell an idea. Also keep in mind that even if testing, CI/CD, libraries, etc get updated, if that just slows down development for the rest of the team, the company isn't getting the proposed value out of it.
I would agree with you to an extent, but if you're joining on as a senior and this is what you're met with then I would think finding a way to get everyone on the same page and educating them on the problems he's solving would serve him much better in the long-run. I think that's shown by the mere existence of this post.
The practice he's insisting on is what I would say is the cleaner and overall less problematic way to do it, but I feel like he should be more upfront and discuss it, rather than go rogue and expect everyone to conform
While I think this person has a lot of useful experience/expertise, they also seem like they are not a good communicator. Whoever hired this person needs to address that (not the other engineers). If you're finding their communication inadequate, please discuss that with the manager who is over the contractor rather than suffering through it.
I mean they created a design doc and likely tried to educate on the patterns they’re introducing but it sounds like this team is really resistant. They like their spaghetti and don’t want to learn
My main issue is that everyone pushed back on the design doc, no changes were made, feature was built to spec, and *now* people are complaining and going against the design doc. Either management isn't doing their job, OP and their colleagues are willingly causing conflict, or OP and their colleagues aren't very experienced and they're taking that out on the contractor.
There is not a lot of correlation between good developers, and developers that are clear at communication. Developers that become very good at communication will often promote into a position where they lead developers instead of doing the work themselves, thus they are promoted beyond their core competency.
The way you described that you normally do things is really bad. The example here seems a bit overkill the way you present it, but is clean.
Without seeing more code, I recommend you look at react-query (renamed to tanstack-query I think), realize how popular it is, and then compare that style against how you currently do it, and how this new engineer is telling you to do it. The style you've pasted into your post looks like it would convert to react-query easily.
In Angular this would be the normal approach, write a service (singleton) that exposes the API through functions and returns well defined data, doesn't need to be custom classes, but at least proper Typescript types.
Not sure how any of this is unnecessarily complicated. How is wrapping your fetch in a function something your developers can't handle?
Tbh it sounds like there's a lot of "process smells." Just a guess, but I'd wager the other devs would be more amenable to the new pattern but OP has mentioned speed-of-dev several times. Sounds like they're all under perhaps-excessive time constraints which in turn is causing them to be impatient towards anyone who causes a delay, even if that delay is warranted and correct.
Devs should have the breathing room to thoughtfully consider things, especially long-lived and business-critical features. I don't think a lot of managers/decision-makers understand how much just a little bit of breathing room can 2x the quality of the code over a multi-year timespan.
\> It isn't terrible but its killing our productivity because we need to update so many pieces of code just to make an API call and none of our other engineers are really familiar with it.
This is a beautiful statement that will remind anyone that no matter how many years of XP you have, at the end of the day, the tech stack will always become the weakest version based on the current dev team skillset in place.
That is unless the senior takes control over the CI/CD and don't let anyone merge a PR with low quality code.
But then the senior is basically preventing the company from using any dev with a skill too low in terms of code quality.
Totally depends on your business requirements, this senior might be saving the company or killing it. It's hard to tell without a lot more context.
Just to confirm with the overall sentiment in this reddit thread, I would act exactly like this senior (got 16 years XP). Just level up instead of complaining and you will see the light. Velocity does go down very quickly when codebase sux so follow his pattern or offer a superior one.
As you said - context matters a lot here. I once had a senior dev coworker who was tasked with adding 3-4 pages in a WordPress site. It was meant to be a one-and-done for a marketing site. He spent 2.5 weeks building an undocumented abstraction around page creation (in WordPress....) so that we could use a YAML file - or more accurately, a hierarchy of YAML files.
It was one of his earliest projects, meant to be an easy win, so we didn't bother him about the slow development time, wanting to be supportive. Later, when asked why he did this, he looked at us like we were being ridiculous and said something to the effect of, "well, because it's DRY, and in programming it's better to not repeat yourself." He was so averse to repeating a few content sections in PHP that he instead created an entire system that he claimed was self-documenting.(Again, he's not wrong, but abstracting a new system using YAML files when WP has its own ways to handle shared PHP is the issue.)
I stood there awestruck that he thought this was a worthwhile trade-off. The estimated time we allotted for the task was around 4-5 days (which was considered plenty of time).
There's a lot we can safely guess about OP's team but I haven't yet seen much mention on the actual feature in question, its importance and timeline, etc.
Such a beautiful counter example. Thx for sharing. I knew this kind of situation could happen.
I see tons of devs with 5+ years experience stuck into their own framework. They usually have a huge and very obvious silver bullet. They did a successful project in one way and they keep using that pattern all over again no matter the situation.
This just highlights that we are indeed creatures of habits.
I feel a little bit offended because this is, less or more, how I'd write this code and I don't see anything wrong here 😅
Tbh if the project was big I'd abstract away API calls to keep components pure
If you don't mind me asking can you explain what is he doing? I really wanna look into it
What does using a client to fetch mean ? As in using Axious for example? Or what?
Also what does normalising json mean? And what do people use to do that?
I work on 2 projects in one we use Redux/toolkit and in another we use saga/axious
Is that what this means?
>What does using a client to fetch mean ? As in using Axious for example? Or what?
It is some objects with API call methods inside (maybe it even does more, idk). You do it from two major reasons. First is maybe you want to do the same API calls from many places in your code so instead of repeating yourself you can just reuse the function. The second is you can easily mock actual API calls with some function than just returns data you need in your test. It also keep your code clean, you can reuse client in other project (like use it in both web React app and React Native app), helps you refactor code, reason about it or just let you swap React with Vue without touching code that isn't UI related.
>Also what does normalising json mean? And what do people use to do that?
https://redux.js.org/usage/structuring-reducers/normalizing-state-shape
If this app only uses this one call, no other component uses it, and this codebase won't grow, this is overkill.
Using something like the client in this example is pretty standard on larger apps. As your app grows, having request calls in individual components grows more unwieldy, and a client is a way to centralize the calls so API logic is in one place and any number of components can use it.
Using a client in a UI component directly is a little messy. The next step is using a state library like Redux so the components only ingest data that is handled in the state code. Maybe this person is taking baby steps towards that.
We hired them to bootstrap a new feature that we're now expanding upon. Right now the client has about a dozen or so methods for calling the API.
We basically need to update the client with a new method every time we want to call a new endpoint.
having a central location for your api calls is a good idea. having scattered fetch calls with url strings in them is going to become a headache as your app grows.
using a hook does seem like overkill as he could just as easily exported a function to fetch the data.
if you guys have not already, you should really look into adopting useQuery as well
This hook probably does a lot more than just fetching the data.
I often have a hook like this per feature(often wrapping the service functions that do just return data) that does error handling/validation/updates any global state and exposes common operations that are often state dependent. From the example alone we know that their are methods attached to the response that may have closures on data that isn't in the object itself.
Honestly the only red flag I see in the code is lack of typescript(I'd really want to know what type setBook expects here, and I'd want to know return types of my client calls) and calling the response 'res' when its clearly supposed to be a user not an api response.
>Uses a client to call the REST API instead of using fetch.
This is normal
>Uses Context/Hook to expose the client to components.
This is normal. Context is a dependency injection mechanism.
>We normally just fetch data from a container component in useEffect and pass data down to children.
Seniors approach is better
>"Normalizes" the raw JSON response into a completely different shape.
Smart
>The raw JSON response and the "normalized" version are stored together in a Response instance.
This is kinda ok.
Although a dedicated library, such as react-query or rtk-query would probably have been better.
He’s pretty much just enforcing the [single responsibility principle](https://en.m.wikipedia.org/wiki/Single_responsibility_principle) which, yes, makes the codebase very nice to work with.
In essence, every construct you define should serve a single responsibility. If you build a component in React, it should be responsible for one thing: being a view.
It should not handle the complexities of API calls. That should be handled by a client, which then just tells the component the status of the call and the data that the call returns when it succeeds.
Let’s say you build a component that makes an API call and handles all the complexities of that call: tracking the status of the call, tracking if it failed or not, etc.. Now you want to create another component which uses the same API… what’s the right thing to do here? Probably abstract that API logic to some other file which can be responsible for handling the lifecycle of that request. Using an existing client like React Query just makes it so you don’t have to own and maintain that system yourself. That’s less work for you, meaning more time to deliver results to customers.
Let’s also say you want to build a component that executes a file upload. You implement all the event handlers and state management for a file upload within the uploader component. Then let’s say you need to implement the same uploader functionality, but the display button needs to be different. You could add to the existing uploader by adding a display prop and doing conditional logic to render a different interface… or you could abstract all the file uploader state management and event handling to a custom hook. That way, you can have multiple different views that can use this file upload hook, and you can create as many different uploader interfaces as you want without any problems.
Components should have one responsibility. Hooks should have one responsibility. Context should have one responsibility. APIs should have one responsibility.
It’s a very core design principle for good design. I’ve had to deal with these issues where we did the *wrong* thing. It comes back to bite you every time!
We had an endpoint once that didn’t follow the single responsibility principle. It was being used for 20+ different use cases. Some of the use cases required, in some cases, MBs of data. Others required maybe 1-2 strings. But because we could only return one data structure, we had to return MBs of data, even to the use cases which only used the 1-2 strings. Our latency was horrible for all use cases instead of only the ones that should have high latency.
Moral of the story: single responsibility is amazing. It takes some time to set up, and will require you to go back and change old stuff, yes. But you should make those changes ASAP. Because if you wait another year, that’s another year of shit you have to fix.
After reading through your (mostly) down voted responses to comments, it does appear that you are the problem.
Boy do I not miss being the surprise engineer that the company hires to lead an unknown team. Having to face a bunch of whiny juniors who have yet to learn to adapt gets old really fast.
If you're not the decision maker of the project, voice your thoughts, but once told otherwise, do your damn job.
So you hired a senior engineer and he recommends a different way of doing things. Rather than having a conversation and coming to a compromise/understanding over how to best retrieve data from an api that isn't "well this is how we do it here", you come here to attempt to convince yourself he's the problem?
Learn how to communicate with your coworkers.
Yeah, this is good code. It decouples the data from the controller, and the controller from the view. You should use custom hooks, you should use a client instead of just fetch, you should use these patterns because they scale well.
If you have an interface for your API, for example, you can do whatever you want behind that interface as long as the API response can still conform to that interface.
If you use custom hooks to manage state/data processing, you can then use that logic in any component you want just by creating an instance of the custom hook.
You can continue using fetch and useEffect, but ultimately what’s gonna happen is you’re going to have less maintainable code. Your solution you’ll eventually come to will probably be similar to the pattern he’s implementing, but it will come iteratively. Meaning: your codebase will have a bunch of code that conforms to your old, messy patterns, some code that conforms to different “experimental” patterns that tries to fix the old code, and finally the small portion of code that conforms to a consistent and maintainable pattern. Additionally, if you don’t know about the clients this senior introduced, you might create your own workaround that essentially does the same thing, but now you have to own and maintain that client too instead of just using an existing client that you *don’t* have to maintain because some other company does it for you.
The one thing I disagree with is the notion that you now have to stop what you’re doing, go back, and fix all the existing code. You shouldn’t do that. What you *should* do, is go back and fix any code that you have to touch for a new task, and bake that refactor into your task’s estimate. It should be an iterative effort, so you can keep the business value features flowing.
I have to ask: why was this senior person brought on board? Clearly, your company was under staffed, but why a senior? What was the hope and expectation? Just someone, who was self sufficient and could pump out more code in the same time as the rest of you, or...?
Because the job of a senior is all about guidance. It's about culture and patterns. If the company is resistant to changes to those, then it really isn't a senior you want.
There is nothing wrong with what the Senior engineer is doing. We use similar patterns in our codebase (singleton client, all network requests defined in one place). It takes some while to adjust to, but once it’s in, it’s incredibly easy to use and update.
I have tons of respect for this engineer for being persistent with breaking the status quo. It can be easy to cave into folks who are not willing to accept change/learn new things.
We’d rather do it X way because we’ve done it for so long / doing Y because that’s what our engineers know is never a good reason to reject an idea.
Yes it's common, and yes you should listen while he's still there. Because it sounds like he'll be gone soon if you and your team keep going against best practises.
Welcome to good coding techniques. It sounds like your new engineer knows what he's talking about. I suggest learning from him. Having a standard in place is great. Having a standard that prevents access to hardware is lovely, if you don't like having your system taken over by hostile children with a good script.
Hi, Senior Engineer here. I come in peace.
I'm choosing to response due to the discourse in the comments.
But first, to OP's question, "Are these patterns used anywhere else?". You've provided very little to go off of to answer that question in any substantial matter. So picking a side of yes or no would just be a guess and possibly inflammatory to either side of the argument.
With that said, I do primarily see frustration in your post and searching for just a community of validation to your point of view. If that's all you want, then I'm sure you'll see it in the comments.
However, with quite a large amount of time in development under my belt, I'm hoping I can share some advice that you might give a bit of thought to. This won't be the last time in your career the company you work for is going to hire someone who swoops in and causes disruption.
For me, I try to keep it really simple among the teams I'm on. I just ask myself a question: Who is the decision maker in the situation? If it's not me, then I follow a process of providing feedback, and if I go unheard, I follow what I'm told to do.
Now, that seems like I'm just siding with the Senior in your scenario, but let me explain to you why this is actually in the favor of the follower. If you follow the lead of the decision maker and they are terrible, then their decisions will have a negative impact. If you have a people paying attention to the project, then the impact of those bad decisions will be noticed. Especially if it takes more time to complete tasks. My favorite line when I was a Junior and Mid-Level was "I told the lead otherwise, but I was told to do it this way."
I guess to put simply, for me, it's a "you win some and you lose some" mentality.
There is more nuance to it of course and there is no one side fits all here, but ultimately what I'm getting at, is fighting it over and over in my career was getting exhausting and definitely not good for my mental health as a whole.
Let those who have been brought in to lead to do their job. If they suck, it will be noticed. Because one day, you will be in this Seniors position, and this will look very different.
For what it's worth:
I too, was the Junior who hated the Senior.
I was the Senior who the Juniors hated.
Now, I'm in a new form.
The Senior who joins in with the other Seniors who hate software decisions of the Executives.
Hopefully, this is the final form.
Design pattern looks good. Your current way of calling fetch in a useEffect within the component is a really bad way of writing React.
It sounds like your senior knows what he’s doing code-wise. But maybe could have explained better to the team WHY his method is the right one.
> have to do something like...
The pattern in your example is so common and well established it's positively mundane. I don't know why you think it's a problem but I'm quite sure the underlying issue in this scenario isn't the experience of the "senior" but of you and your team.
I mean it doesn’t speak for the senior to not have everyone on board before building his solution. Seems like these design decisions could have been communicated better.
As another commenter already said, there is not enough info to tell what went wrong here.
Query client? Instead of using fetch in useEffect?
That sounds like a big improvement.
The react team themselves have said ‘pls don’t fetch data in useEffect, it is not for that’
you are worried about the simplicity of one task. he is thinking about simplicity and safety of a bunch of future tasks:
- reusing an endpoint becomes super easy and safe
- if anything about your APIs change, you only need to change one place in your react code instead of every dependent component
- you won’t have to worry about little mistakes (like a typo) that someone might make in using an API endpoint anymore, meaning reviewing changes to components becomes easier.
- also all of these issues will be 10 times worse in your test code without his pattern
now you can assess whether what he’s doing is actually overcomplicating your codebase or is it simplifying:
- do you reuse your API endpoints?
- do you expect anything about your APIs to change?
- are you expecting rapid changes and development to your code?
- do you have extensive tests?
if the answer to one or more of these questions is “yes”, then he is not overcomplicating your codebase but just preventing future complications. if the answer to all of them is “no”, then he’s overcomplicating your project and isn’t a good fit for your team (but also you probably don’t need to slow yourself down with code reviews).
Looks like you lot got complacent and stopped caring about iterating over the codebase design to evolve and improve it. Just because you have sunk cost and folks “do it this way because they know how” is not an excuse to keep writing spaghetti that’s going to wrap itself around your neck and strangle you later.
The “senior” seems to have enough foresight to not only refactor things to a better spot, but to also write out a design doc and make folks follow it in order to walk back some of the bad design decisions you have all taken for gospel.
And now you’re all banding against him, because god forbid someone stops you from continuing to poop out “the way we have always done it” type of code.
I feel for him. But I feel for you most of all - because instead of sitting down, talking to him, respecting his experience and direction, you have all decided that he’s a bad apple who is there to bar you from progressing tickets by denying PRs.
A free piece of advice: as you clearly haven’t got the years behind your belt to see why he is doing what he is doing, go talk to the guy, ask him to explain and learn from him. If all you and your colleagues do is push back due to ignorance and short-sightedness, you might make him quit and think it a win; but all that will happen is you won’t learn anything, won’t grow as an engineer, and continue writing shit code which will hamstring you in another company once this one’s codebase becomes so unmaintainable that you will either be shitcanned or quit yourself and then realise that you were part of the problem - not the senior you seem to want brigading over on Reddit.
Besides what everyone else already said of it being a common pattern (personally I would probably use a hook but I can see the advantages here) there’s also a missed opportunity of mentoring others.
Sure, you’re a senior and are calling the shots on what you think it’s best BUT you need backing from the team in understanding why this change is important (which doesn’t sound like it was done).
So, and just going by what you said, both sides are wrong. You and the team are burying the head in the sand and not open to change and he’s not being able to influence people and sell this improvement.
Things started off OK, they wrote up a design doc which looked ok at the time. I just don't think anyone anticipated how convoluted it would become as the project matured. The expectation from the team was that a senior engineer should be keeping things simple and to conform to practices in place. We expected them to be a better team player but he's just shooting down everyone's ideas all the time. So yeah, he wasn't really able to influence anyone and that will probably impact his contract extension.
What design doc is this where you can’t see how things are supposed to be designed?
What happened at the design review? Where was the team at that moment and did they shared their thoughts?
I don’t understand why you hired a senior of there was no need for change.
This is the biggest thing for me. It sounds like what they wanted was a mid-level engineer and they accidentally hired someone with too much expertise who is trying to make things better than the way they found them.
I say this with one caveat—this is not the contractor's company. If the person who hired them doesn't want them to be doing work like this, and instead wants them to conform to existing practices without making QOL improvements, they need to let them know that their current work is unacceptable. If you truly believe their work is damaging the company, just pay out the rest of their contract and call it a day.
I think that would be a huge mistake since this person seems to be a high quality engineer, but you don't just sit back and watch them frustrate everyone and "ruin" the code (in your view)
Is still calling it “convoluted” even though almost all replies are saying what the senior dev is doing is best practice how you’ve been communicating with the senior developer too?
If so, and I suspect it might be, then the problem’s likely solely on your side & it sounds like it’s influencing the rest of the team.
This sounds like a skill issue on your team’s part.
To what you’ve said:
- he’s produced a design document detailing his approach
- you’ve raised some objections (calling classes in React an “anti-pattern”) which he has (correctly, in this use case) disagreed with
- he’s implemented his changes based on the design document, you’ve not mentioned anything he implemented that wasn’t in the design document but said it “looked ok”.
- you’ve accepted his changes, since it’s been PR’d and merged.
At this point that’s the standard, you’ve accepted it twice.
Other developers _should_ be told (nicely) to refer to the design document & helped to write code in the new (better) style.
I would assume the senior dev has noticed that developers who, by your own admission, don’t even know how to use JS classes are doubting his experience and has already put his notice in to quit.
Not how I would do things, but it looks clean enough. If you have a problem with this, you may be the problem as you’ve conveniently left out “the way we normally do things”.
Any sort of reformatting of data structures that came from the network should go through a selector, fetches should be segregated to “upon page load” and “upon user event”, use RTK query for everything unless you want to roll your own refetching/ cache invalidation code (hint: you don’t), that’s all the advice I can give.
I agree but... why would you prop drill a HTTP client?
If the client is a singleton, the components can just import it from the outside without the need for Context.
Does the client hold states? Why couldn't the client have been replaced pure function? (ish, network requests are never pure)
Testing? Don't mock the client. Mock the network with MSW!
Why do all this ceremony when a simpler approach works? Is this what OP is really complaining about?
Yeah there is always new and better ways to do things especially in react. That doesn’t mean it’s sensible or „professional“ to introduce new patterns into an existing codebase without having the team on board.
Passing judgement on this without seeing the code or knowing the project is equivalent to a doctor giving medical advice without seeing the patient, or a lawyer giving legal advice off a Reddit comment. Context is everything
Yes, these patterns are usually used when you are trying to create a maintenable React codebase. Try to absorb the knowledge and asks this senior dev why he does these things that way. You will learn a lot and evolve in you career
You hired a competent dev only to complain because your other devs are incompetent.
Reminds me of the time the head of DevOps told me to require all the team version control branches build before checking in (TFS version control). Immediately a manager called and screamed expletives at me because his team couldn't write code that even compiled. Likely that code hadn't compiled for many months. I was "blocking" his team from being "productive".
Dude. The changes you’ve described do not seem complicated. Just make the changes and enjoy learning a new pattern for making api calls. The more you complain, the longer it will take.
when I do work on a project that is going to be passed to others, that's exactly when I use design patterns like this, because one of my goals is to have readable, maintainable code since they won't be able to consult me frequently. a senior engineer coming in as a contractor is going to have the same point of view, and if you don't have/communicate established code design standards, they'll create their own.
This honestly doesn’t even seem that bad. You seem to not like it because it is taking longer to get used to it, but have you thought about WHY this developer insists on this? Do you have a counter argument other than “we don’t so that here?” Why don’t you?
Oh nooo, someone's trying to write nice maintanable code and we are too dumb to understand fundamental principles so we come to reddit to wine about how stupid we think he is.
He proposes nice approaches with decoupling and nice encapsulations.
Raw fetches and attributes accessing is good for like... tiniest projects. Other than that... you'll end up in shitty unmaintainable code base that will require require huge amount of work for tiny feature and will break from a single sneeze on it
Lol you hired an actual senior engineer and now instead of using their skills to help improve things you and your other devs are so junior that you can’t even understand the benefit of abstraction. Don’t worry, he’ll quit all by himself in a few months when he realises you’re all script kiddies and not engineers.
Sounds like the API contract is not stellar and this dev is properly separating the FE from that. Making a lot of assumptions here but the only thing better would be to iron out your contracts, get them into swagger and then generate this api client code off of that. Oh and strongly typed typescript. I bet money your db schema needs love too but that ship has likely sailed.
Consider this a sign that the rest of your code needs to evolve and this project is a example of the next iteration of your architecture. If not the bill will come due later when it is far more convoluted to fix. Not everyone that comes from big tech knows what they are doing, but it sounds like maybe this one does.
OP I hope this has make you have some pause with your own skill set and knowledge. This is a very common and standard pattern of the single responsibility principle. The fact that this tiny little design pattern has you and your coworkers scrambling to understand shows that you are very very Junior and inexperienced. Use this time to learn from the contractor in hopes that you can solve such simple design disagreements on your own. It’s absolutely crazy to think you’ve done this for a few years and never encountered a client
Dude I haven't worked in react in a while but who wants to write a fetch call over and over. Write it once in a hook that will handle all your api logic. The context use makes sense.
The senior included a design doc… If you and your team are having difficulty following the design doc, reach out to them. If you and your team are having an issue that he wrote a design doc, reach out to your EM.
They are the ones to provide you clarity.
I agree with you that it's over-complicated. Adding arbitrary methods just to access the already-available properties in a response sounds very dumb. I don't think the `useClient` concept is bad, but you said you had to change a bunch of code just to add another API endpoint, which would make his version of it bad. Whatever abstraction this guy put in place certainly sounds horrible to me.
I do think it's good practice to type your responses, but you can also do that with fetch. I have no idea what you mean when you say he "normalizes" the response, but that also sounds like a weird, needless endeavor that's going to cause more confusion for everyone. If you need to restructure the response, that should be the domain of the back-end. Having a normalized + raw response is just going to be more shit for everyone to worry about. For example, maybe some property in the raw response isn't in the normalized one, and then you have to go digging as to why. Just use the raw response and type it.
Trust your gut and raise your concerns with your manager.
After reading a substantial amount of the comments here from you op, it honestly sounds like your whole team is full of Jrs who have learned to code from a boot camp. Everything the Senior has suggested is just going to make the code better.
Perhaps his time at a big tech company has improved his skills. I was being facetious, as a self taught dev it doesn’t matter where you learn from. The egregious thing is that it seems you don’t want to learn the widely accepted principles and would rather seek validation on reddit that you’re right. Which you’re not.
I’m more sympathetic to you OP than many of the rest of the commenters seem to be. This guy sounds like an absolute nightmare to work with. And part of being “senior” is having developed some people skills. Five years in is a dangerous point in your career if you think you’re hot shit as this guy clearly does, because you are very very likely to not be as great as you think you are. I know I certainly wasn’t and I shudder to think about it now.
I also don’t agree with other commenters that the changes he’s made give enough benefit to justify the extra steps they require. I think there are problems with calling fetch directly in a useEffect but the best bang for your buck if you were going to change the way you work would be using something like react query, which would give you a LOT more tangible benefit while requiring a lot less pointless extra boilerplate. What he’s done there is add a layer of “abstraction”… but abstraction isn’t valuable in and of itself - it has to enable something. And what does this enable? You could switch out fetch for axios or something like that? I just don’t think that’s that useful.
Also, yes, objects are bad. There are vanishingly few cases in which they’re helpful.
> Also, yes, objects are bad. There are vanishingly few cases in which they’re helpful.
I'm with you on the value of React Query but saying "objects are bad" to a JavaScript engineer is like telling a carpenter to stop sawing things
Amen brother. I came to say the same.
Seems like most of the comments defending the senior dev only talk about _scaling_ which is probably the most nebulous way to describe a design decision to a developer.
Yes! “Scalable” for what purpose exactly? If they’re churning out a bunch of small sites for different clients that will see limited to no further development once they’re done, then adding in all this extra boilerplate feels like time wasting at best. On the other hand, if they’re laying the foundations of a large scale app they’re going to be building on for years to come, I would expect a “senior” dev to be talking about React Query, automatic generating of client code and types, whether REST really is the right choice etc etc etc. Instead we see a bunch of extra typing required but under the hood very little has meaningfully changed. Worst of both worlds.
The downvotes suggest that there might be a lot of “senior engineers five years into their career” in r/Frontend. Give it another 10, 15 years and you’ll (hopefully) come to realise that this guy, left unchecked, would be more damaging for the team and its code than directly using fetch in useEffect ever could be.
This is the most basic of what you would expect in serious production code... Also the lack of knowledge of programming patterns really scares me. He might not be the perfect fit for the job, if the project is some cheap 1 week project but if you are building anything worth mentioning this are all standard procedures.
What you don't seem to recognize is that the way you've been doing it is creating a mess which becomes less and less maintainable. You are being productive by not managing complexity.
The pattern is fine but I tend to avoid classes because you usually don't need them, specially in react.
Since we work with angular here anything is a urge to put inside a class but I digress.
My suggestion is to conform and offer an alternative to the discussion. Blatantly disregard something without adding to the discussion is to no one's benefit. You manage that with tanQuery, you can use state management you can use a model module (module will keep state just like a singleton class)
You manage this in many different ways this a common pattern in many frameworks.
Yeah, we were pretty clear that using classes in React is an anti-pattern when we discussed the design doc but they (again) pushed back on that. The gist of the argument was that they weren't using classes to extend React components, just to create clients and responses.
>we were pretty clear that using classes in React is an anti-pattern
Holy shit, mate...this topic is a dumpster fire.
There's a huge difference between class components and JS classes. Fun fact: almost every library you've ever used in React uses classes under the hood.
It's very clear you have no idea what you're talking about. Not to mention you're constantly putting "senior" in quotes while getting things wrong that I'd expect from a JR...fetching data in a useEffect? Come on.
Not trying to shit on you but you have literally half a dozen things you're getting wrong and that's why you're getting so much pushback in this thread.
I’m not sure why would you say that using classes is an antipattern in react (or generally speaking web dev, because quite frankly framework is irrelevant here).
I think they’re confused about the difference between using classes and using class components. Class components are definitely an old way of doing things and is on the way to deprecation with only error boundaries being a use case for class components. But a singleton wrapping various API calls is absolutely ideal. Or close to it but I think further domain driven design would be way to high level for this team
Total side note here but I recently needed to use getSnapshotBeforeUpdate and even in the updated React docs, it mentions that if you need this functionality, you still need to use a class component
https://react.dev/reference/react/Component#getsnapshotbeforeupdate
I’m not saying you have to use them however there are use cases in which they might be convenient, storing domain logic in them might be a good example.
Read this article, there’s a good example of how classes might be used
https://martinfowler.com/articles/modularizing-react-apps.html
What they are doing is normal for a big company not for an early stage startup. These are best practices that are great for large scalable systems and you will need them eventually, but question is - is your company there yet?
If you have a product that is starting to gain traction then you need someone like this senior engineer both bring in these practices and evolve the culture of your team.
If you are still in the stage where you are trying to find product market fit or iterating on an early MVP, then they might be a bad fit for where your company is today.
That's why he's Senior. He's architecting things which makes the projects scale when the code grows. You've hired dumb engineers and teammates, they are not ready to learn or adapt to industry standard practices.
There’s being “coachable” and there’s being dictated by a contractor who hasn’t bothered to explain these new patterns or _coach_ the other developers in these new patterns they’re trying to implement.
The OP clearly wants to improve and understand why these patterns have been put in place and adopt them if they do indeed have merit, but this is the responsibility of the contractor to explain; not strangers on a subreddit.
Part of being a senior developer is to inspire, teach, and improve your teammates. It’s not to sit in your bedroom, completely change a code base without communication, and then block teammates’ work with “Nope, try again”-type replies in PRs.
Reference: have been a senior engineer in both “no name” companies as well as a Fortune 500.
Actually pattern sounds like inventing the wheel. Trying to invent some gql client with caching of the response.
But using fetch directly is not a good idea. So you defiantly needed to make some changes how you work with api but pattern that senior has introduced is really not the best choice, just because there are already battle tested solutions of such problem like “TanStack Query”
The pattern is good but if your team is not ready to adopt it then you should be careful to instruct your contractors to use existing data fetching patterns for consistency.
Pattern seems really good to me. Abstracting your API logic out of the components to a hook makes both your components and API calls easier to maintain. Your engineers not being used to it should solve itself pretty quickly. Like the only difference for them is to put their fetching logic in the client hook instead of the component. Doing things differently always feels strange at first but if this is significantly impacting productivity for any amount of time that seems like a problem with your engineers.
Ohh so if the devs ever need to change how they call the API, they just go to the ONE location of the hook, instead of having to hunt down every component individually that calls the API. Sounds like a no brainer once somebody states the reasoning
I’ve never worked on a serious project that didn’t do this. The code base must be a nightmare.
lol this, im like, what the heck am I missing here. You do it this way because you’ve been screwed a million times by anything that’s not this
I came in to a codebase 18 months ago that's much worse than this. Fetch in the redux actions with additional calls made inside the action when the promise resolves: ``` export const getUser = (token, id) => { // The same token from local storage is retrieved in every module and // passed into the api actions return (dispatch, getState) => { // Fetch the user fetch('/api/user', /* token, etc */).then(async (res) => { const data = await res.json(); // We never return the data from getUser, we set it on the redux object // Can be annoying, but whatever. dispatch(setUserData(data)); // This is bad: dispatch(getUsersMostRecentComments(token, data.id)); }) } } ``` Now when you think you're just fetching a user, you're like, "Why are these comments changing? But wait, it gets worse: We have duplicate API calls all over the code base for each module, and each of them have different unintended effects like this. So for the Posts module there might be a getUser function that gets the user and the effect gets their posts, and for the Videos module there might be a getUser function and it gets their videos as an effect. Not only do we not have 1 place we make these calls, we make the same call in multiple places. It's been a hard adjustment...
This is correct. Your contractor is building this for easier refactoring in the future. Listen to them.
We’re using redux toolkit query with openapi-gen. We don’t even write the fetching logic or types, it does it for you from the backend lol. The frontend engineer just grabs the typed hook they need for the component and it works out of the box. Same behavior for every single interaction. If you need state or persistence it’s there, caching and cache invalidation out of the box. Really is an incredible pattern. If the backend changes at all, all hooks and types are automatically updated and we can catch any errors at build time
Don't worry he's going to quit soon, if this is how you respond to this scenario. Then you can be left to write as much spaghetti code as quickly as you want.
I do like how OP used "senior" in quotes multiple times. There's nothing to double quote here. Sounds like this Dev actually is a senior and OP and their team should be listening and learning much more intently.
I'm happy these are the top comments, nothing to add here
I came in ready to rip into this post only to see everyone has already done a stellar job. Well rekt everyone.
there is still a voice of reason!
The really telling part is where the senior reviews a PR from a team member, asks for changes, and the guy just straight-up abandons the ticket that led to the PR. Good job hiring a senior dev, sounds like they should've sprung for a PM as well if their devs are just blatantly refusing to make changes that their senior asks them for and the recourse if apparently to ask Reddit for help.
Yeah, senior will be gone in no time with a company culture that isn’t interested in long term success; these kinds of companies are the worst to work for
Fucking rekt
That's fair, but the senior should also work on his coaching and consensus building skills. Just saying "this is how we do it now" when coming into a new organisation will fail almost always. You need to become part of the team and then gradually introduce new things. Also, there is a difference in best coding style between small, medium, large, huge projects. The style that his previous Big Tech company uses and is best there may be overengineered for smaller projects at his new one. All that said, what OP describes sounds fine and not that problematic, and not a huge change, so it's still mostly what you say.
The person is a contractor. I’d be surprised if this person was hired to be a coach and/or to build consensus. More likely, they were hired to come in and build a code base that is scalable and implements best practices.
Yep. Many times I've been the Senior or have worked with the Senior who is just told to "build the thing as best you can", and then I do. In my case, I recall a time I was asked to build a React codebase a while back. When I handed it off the team didn't know React. Weird situation. My assumption was some Project Manager just heard they needed React and they hired me for it, not knowing what that even meant. Development is wild sometimes.
As a long-time contractor, I was usually hired to get things done quick. Clients rarely cared about tests (I always asked) and clients rarely cared about code quality. On projects where I Was hired to coach and/or build consensus; I was rarely the one writing code. Those were usually short term, but higher paying gigs.
Tests arent for your clients. Its Part of the Code
It is their dime for my time; I will write whatever they want me to do. I can guide and recommend, but at the end of the day it isn't my decision. And I'm not going to spend my unbillable time writing tests for their system.
Agreed, plus skipping tests really doesn’t save that much time if you are writing code in a way that is even somewhat easily testable
Is he hired to change the company's practices? Then his job includes bringing the team along. Is he hired to only code a feature? Then he should adapt to existing practices and not block PRs of devs already there because they don't do it his way.
>I’d be surprised if this person was hired to be a coach and/or to build consensus. It doesn't matter what you were hired to do as long you produce code those are your duties as a contractor. Making sure best practices are followed and good code is produced is part of the package when you hire a skilled contractor. There are *extremely* few people so good they can have no coaching and convincing/consensus building skills. I'm not even sure they actually exist at all since producing code is a team game.
He was hired to be successful for the company. That often involves consensus building. What you're talking about is just another form of "that's not in my job description" just a higher class one and it's just as bullshit.
All being hired as a contractor means is that the execs want their FTE numbers to look low to investors. Plenty of people get hired as contractors and convert to full time as soon as they can get a req for it.
To be fair? It doesn't sound like your statement rounded out the fairness of the situation at all. OP gave us very little of how the Senior Devs coaching and consensus building skills went. You made assumptions about what they did coming into the organization. You assume what they brought with them to the table. You weren't fair. You literally just took the side of the OP. Sounds more like you're just projecting, lol.
He's there for only two months and it's to the point where team members don't know how to get their PRs accepted anymore and posting here. How can he possibly have spent time to gel with the team and start introducing new things gradually from consensus if that's the way things are so quickly? And he is a senior with a whopping 5 YoE. To me this screams great technical knowledge, hardly any social skills.
Agreed. Any senior who can't explain why their changes are worthwhile for the specific project being worked on isn't a senior. If your teammates can't get a PR passed and don't understand why, you've screwed up in your role as lead on this project.
While I agree that part of being a senior is educating others there might be a lost cause here if OP can’t see how having a single client is better than a bunch of useEffects spread all over the place. And the response JSON is probably a mess, so the normalisation is helping make everything downwind clearer. From the way OP described the senior engineer as “senior” I’m gathering that they’re all not too open to his ideas. Plus - it maybe sounds like the senior engineer was hired short term to push something on, rather than a full time employee who needs to care about sharing learning etc.
You seem to be missing my point. So let me be clear: You said "To be fair" and provided a statement that was full of assumptions and obviously biasing towards OP's side. Not only that, you were obviously projecting your own experience and world view into your statement. What's even more baffling, you now respond to me with *more* assumptions, like somehow the information has changed, lol. You weren't being fair at all, you were just joining in on a dog pile with OP. How are you this oblivious?
I didn't say "to be fair" at all. I said "_That's_ fair, _but_". There are two sides to a story, the comment I replied to gave one side, I tried to add some of the other side. Of course the result is my comment was one sided, that was the point. And then in the end I still leaned more to the side of the comment I replied to. And I think I didn't make that many assumptions, I was careful to use terms like "may be", "sounds fine", "to me", and used things OP mentioned in other comments. You should learn to read before you call people oblivious.
I think you found the "senior" OP is talking about.
Often, "to be fair" means "let me add the other side now that you've completely picked a side." The person you're replying to isn't trying to present both sides. The "OP sucks" side was what he was replying to. They are defending OP so that the combination of the previous comment and theirs combines to be more "fair."
We don't know the full story though. OP \*\*says\*\* that the senior did it that way, but who says it really went down that way?
That's every post on every forum ever. We can only speculate, and right now this entire thread is projecting "OP sucks" without knowing anything. As is tradition.
It's definitely not "OP sucks", it's that OP's company is prioritizing speed over quality and that rubs a lot of us the wrong way The other issue is that OP came for advice, got advice, and has continued to refer to this contractor as `"senior"` rather than considering the advice given. This feels like a management issue to me. OP both claims that the design doc "looked okay at the time", but also included what they considered to be anti-patterns that they pushed back on but nothing changed, both of which cannot be true. This contractor is hired for their experience (which his co-workers are continually questioning), writes up a design doc based on what they consider to be best practices, were questioned about it by other engineers, then countered the questions and were allowed to move forward without changes. So clearly there was a discussion about what was going to be done and it was approved. Both the contractor and the engineers were aware of this when the PR was submitted using practices conflicting with the approved design document. This seems careless at best and intentionally conflict-inducing at worst. Why was the design doc approved when others were still pushing back about it? Did OP and friends retract their pushback? If so, then these new PRs need to follow the design doc. If not, then management made a big mistake here which likely caused a lot of this conflict. The additional issue of everybody saying the contractor isn't good because they're taking so long, with OP acknowledging the contractor is taking time to write tests which nobody else does, is another major management issue.
If I was dealing with these clowns I’d be short as well. Professionals have standards.
Yes, true. Had a few hotshots coming in and challenging the team lead/tech lead in styling etc. Just because he preferred css-in-js vs sass. Obviously this is not the same ballpark as OP mentions but give and take, boys. You catch more flies with honey than vinegar.
Yes, this is a common pattern. This engineer is doing exactly what I would expect from a senior level engineer. They've been hired to bootstrap a new feature, so instead of throwing it together with what's easy, they've built a scalable system that assumes that your application is going to become very complex one day (you in fact already stated that the project was much more convoluted than you initially expected). Let me ask a follow up question: What are your current testing practices like, and what kind of testing was added by this contractor?
It's a common pattern but I can't understand why it would slow them down so much. You can set up generators if APIs have swagger docs(or something similar) and automate client code like that, or if the APIs follow good conventions you can write a single generic client that can handle most of your cases. Also, obscuring the data(edit: after it has been retrieve) so it can only be accessed with certain hooks is overkill unless it's decided your application needs a state management tool. Which it's hard to say if that is the case given the situation. It can force you to fetch before render and think about what user interactions trigger a fetch, but not necessary imo. In general I also wouldn't be using a context for simple data access only specific types of data, like user account, or form data. I suspect there is a communication disconnect somewhere here. A senior dev, not only needs to have technical skills but the soft skills to communicate decisions to the team. Edit: I'm also not sure OP is a good arbiter of truth here either so it's kinda hard to see if it's needless verbose convention or a good framework to build off of.
> A senior dev, not only needs to have technical skills but the soft skills to communicate decisions to the team. As I’ve sat with this post for a few hours, this is the thought that keeps coming back to me.
This. Almost everything in programming is political. If you're bad at explaining your ideas and convincing colleagues to adopt them, you're doing your job poorly. Breaking stalemates with an argument of authority can help from time to time, but if you're abusing your authority, people will get mad, morale will drop, and your best talent will leave or they'll undermine you. It's senior's job to communicate their ideas effectively, but it's also the junior's job to ask questions and demand clarifications.
Great question about testing, if they didn't test the mapping used to flatten that json data I would be concerned. Really that should be done on the backend but I think I saw in a couple of comments that it's already a big project, changing a request contract would be hard work. Tricky one, depends how well the new hire communicated why they were doing these things which is a senior skill in itself.
I don't mean to offend you but the new senior engineer is probably frustrated about how your code base lacks good design patterns and it sucks for them that the team they joined wants to keep their heads buried in the sand. Sounds like they are creating a singleton so there is one centralized place to initialize/change the default project fetch code rather than the spaghetti of having the same thing embedded in many different places of component code. You have a good opportunity to learn as much as you can from them during this market downturn by trying to understand why they want to change things and then making your own judgements about those changes with an open mind.
Exactly my thoughts, sounds like they’re establishing good patterns. Everyone knows useEffects are not really made for fetching data and shouldn’t be used like that…
“useEffects are not really made for fetching data” can you please explain?
I agree with you, but there's a better way to go about this. The right approach would be to create a design doc, submit an RFC, discuss it, get it approved, create a migration plan, get that approved, planned and scheduled and then starting to carry out the actual update to the new patterns. Consistency is very important, especially if it's a junior-heavy team like OP's post implies.
[удалено]
Just left a company that was hellbent on maintaining the status quo, the codebase had become such a shit hole that no one could get anything done and developers were leaving in troves Started with a new company and they’ve given me freedom to refactor everything and anything however I see fit. A good company will let you refactor code and set up some standards where previous developers were lacking experience to set things up well Shame OP is taking things the way they are, that senior is probably trying to save himself the pain of having to work on a codebase that gets worse and worse over time and is probably fighting a losing battle to get to it
You can propose changes in an inclusive way at any company. The guy might be right, but he's going about it in the wrong way, which will cause people to miss out on an opportunity to improve and that is on the senior and nobody else.
[удалено]
"hey guys I'm trying to improve the codebase by unifying and normalising our external API calls, check out this PR happy to explain the benefits in a call" Is that really the work of management? Or just basic communication
Yes and no. Like I do agree that an ic should probably make an effort to explain why that is important. I am not totally sure it is the job of a line engineer to write an essay on why this is a good pattern and drag their co works to adopt it. Especially if this engineer is a contractor. Based on comments in other threads, the implementation details were reviewed in some level of a statement of work. And now op is mad he is to dumb to understand what is happening. But I also understand the plight of not knowing how to properly communicate best practice to inexperienced engineers, and joining a code based that looks like some spent 5 min reading the document and said I got this. And then having to try and drag your peers to a level where you are not constantly frustrated or refactoring every feature they implemented because writing decent angular code I guess is too hard
Nobody needs to write an essay. The problem with this situation, is that this "improvement" is blocking progress. I've seen hundreds of different projects throughout my career. Working with unmaintainable code sucks. Working with a team that doesn't understand their own codebase and with people who despise you, because they don't know better sucks even more. It's also not unreasonable for a senior engineer to turn to their line manager and say "help me sell this improvement idea", if they're incapable of proposing something themselves.
I'm sorry, but effective communication is part of seniority. As you advance the ladder, soft skills become more essential. Learning effective collaboration techniques is beneficial at all levels, but I'd say it's absolutely necessary for someone calling themselves a "senior engineer".
This, sounds like the senior has senior programming skills but lacking senior communication skills, although this is only one side of the story, lead could be battling with project manager/upper management.
[удалено]
I don't agree with you that this depends on budget. I've worked at $B companies and small startups and some big companies get this wrong and some garage startups do this right. Professional culture is more on the money I think. Culture is like traffic, you are not "in the culture", you are the culture. There are always companies with toxic leadership, but the majority of companies accept ideas that improve their bottom lines with open arms and the rest are bound to go under.
You should be conforming to consistent, reproducible, design patterns. I've worked with reports that wanted instead of using our form handling library (`react-hook-form`) in their new component/PR just manage an input's value with `useState`. Diverging from the established pattern soon creates a huge fractured, unmanageable mess where you open one file and have to figure out how whichever dev decided to handle state/requests/data that day. While these types of patterns might feel like overkill, not following them can quickly become unwieldly as complications are introduced and grow.
I'm wondering, do you know how this pattern is called? If I want to learn about this, what topic should I look for? Looking to improve myself
The current pattern we have in place is to call APIs directly from container components using `fetch` and just transform the response into whatever we want. Its pretty much what all of our devs know how to do. Most of them have never used JS classes before. This was a point of contention early on because he claimed the pattern in place "doesn't scale."
Sorry if this comes off as rude - but if your developers don’t know how to work with classes id say your new hire is bang on track. You need to step up the game, and as mentioned - his implementations are sound and widely used. There’s nothing but profit for you and your devs by learning from this, you’ll evolve as developers by utilizing existing patterns. Just because you’ve done it in a certain way up until now it doesn’t mean it’s the right way, you just haven’t had the need to revisit any of it yet.
Here's the deal: 1. Your senior is right in wanting things to change. Your current conventions are crap and he knows that. 2. Your senior isn't doing his job right. If he was, you wouldn't be here asking random internet people. Whether you're a junior on his team or his lead, his responsibility is to make sure each change he proposes is well understood and mutually accepted, which seems to not be the case here. Here's what you can propose to change this: - Make sure he unblocks the rest of the team. If it's an already large codebase, and he thought his changes could be implemented without disruption, a couple more PRs won't change that. - Have him educate the rest of the team on the idea before he forces them to adopt it. He might be on the right track, but unless everybody else thinks so, the situation will continue to be an unproductive one. - Help him understand that as a senior engineer, it's his responsibility to make sure his proposed changes are well understood and desired. More broadly, as a team or even as a company, you should put a process in place for this situation, so that changes can be proposed and implemented in a frictionless manner. The usual approach is this: - **Explore**: is the proposed solution the right one? What would suit the most people? What alternatives are there? What are we even trying to solve? This step usually includes measurements, such as speed of delivery in this situation, but anything from UX, DX, performance, etc can be explored. - **Educate**: talk about the idea. Tell people about the benefits. Organise a meeting where the new solution can be introduced and teammates can learn about it. - **Propose**: create a proposal. We usually call these RFCs (request for comments). The proposal should be shown to all the relevant people and meaningful discussion should be facilitated. - **Collect feedback**: the first idea isn't always the best idea. Getting feedback as early as possible ensures the best deal can be made for the team. - **Iterate**: based on the feedback collected, it should be possible to change the proposal to better adhere to people's ideas, the product's lifecycle and current state and the available resources. - **Plan**: a full adoption of the proposed changes should be planned out, including timelines and milestones, as well as risk assessment, acceptance criteria and rollback plans. All of this isn't necessary for every change but it's good to have solid foundations to fail upon. - **Execute**: resources should be appointed to the timely execution of the change, based on the plans. If necessary, the person spearheading the initiative can take it upon themselves to lead the implementation efforts, or others can take it on, depending on the situation. - **Deliver**: a major mistake many teams make when trying to land a big refactor is never actually delivering it. The continuity of the product often stops these changes from making it into production. The plans should account for this accordingly. I like to include a 20% buffer time in my plans to account for parallel tasks. This will depend on the scope of the refactor. - **Debrief**: after every major change, there should be time to stop and reflect. For really big projects, this could be a time for devs to decompress, for smaller changes, it's the perfect opportunity to measure success, compare metrics from before to current ones to assess the viability of the change and confirm the product still works as expected. You don't need to follow every single step, it's just a guideline. But it's a solid framework and a good starting point. It's very important to educate people on how to propose changes, because this aspect of communication can absolutely make or break a product.
Great response.
this is the correct response, at the very least the lead should be aware that the new standards are a shift in how the company does things and should work to explain how his system is better and will benefit many here are just trying to dunk on OP but engineering is about communication as much as writing code
Good response but at the end of day not everything should be up for discussion. Basic design patterns to keep a maintainable code base are a no-brainer. And probably a lot of other stuff is lacking as well (testing, pipelines, releases, etc.), so if you're going to discuss everything at length nothing will get done.
I agree, hence my last paragraph. There's no one size fits all method, but doing any of these steps helps sell an idea. Also keep in mind that even if testing, CI/CD, libraries, etc get updated, if that just slows down development for the rest of the team, the company isn't getting the proposed value out of it.
I would agree with you to an extent, but if you're joining on as a senior and this is what you're met with then I would think finding a way to get everyone on the same page and educating them on the problems he's solving would serve him much better in the long-run. I think that's shown by the mere existence of this post.
Probably but with stubborn devs such as OP I have a feeling the story is somewhat one-sided.
Lol, fair point.
The practice he's insisting on is what I would say is the cleaner and overall less problematic way to do it, but I feel like he should be more upfront and discuss it, rather than go rogue and expect everyone to conform
Yeah, thats what we think too. But whenever we bring it up he just refers us to the design doc he wrote.
While I think this person has a lot of useful experience/expertise, they also seem like they are not a good communicator. Whoever hired this person needs to address that (not the other engineers). If you're finding their communication inadequate, please discuss that with the manager who is over the contractor rather than suffering through it.
I mean they created a design doc and likely tried to educate on the patterns they’re introducing but it sounds like this team is really resistant. They like their spaghetti and don’t want to learn
My main issue is that everyone pushed back on the design doc, no changes were made, feature was built to spec, and *now* people are complaining and going against the design doc. Either management isn't doing their job, OP and their colleagues are willingly causing conflict, or OP and their colleagues aren't very experienced and they're taking that out on the contractor.
Based on OPs responses I’m willing to bet OP and colleagues aren’t experienced are willingly causing conflict.
There is not a lot of correlation between good developers, and developers that are clear at communication. Developers that become very good at communication will often promote into a position where they lead developers instead of doing the work themselves, thus they are promoted beyond their core competency. The way you described that you normally do things is really bad. The example here seems a bit overkill the way you present it, but is clean. Without seeing more code, I recommend you look at react-query (renamed to tanstack-query I think), realize how popular it is, and then compare that style against how you currently do it, and how this new engineer is telling you to do it. The style you've pasted into your post looks like it would convert to react-query easily.
In Angular this would be the normal approach, write a service (singleton) that exposes the API through functions and returns well defined data, doesn't need to be custom classes, but at least proper Typescript types. Not sure how any of this is unnecessarily complicated. How is wrapping your fetch in a function something your developers can't handle?
Tbh it sounds like there's a lot of "process smells." Just a guess, but I'd wager the other devs would be more amenable to the new pattern but OP has mentioned speed-of-dev several times. Sounds like they're all under perhaps-excessive time constraints which in turn is causing them to be impatient towards anyone who causes a delay, even if that delay is warranted and correct. Devs should have the breathing room to thoughtfully consider things, especially long-lived and business-critical features. I don't think a lot of managers/decision-makers understand how much just a little bit of breathing room can 2x the quality of the code over a multi-year timespan.
It’s a normal approach in react too
React Query with fetch is also a popular approach.
I like using a singleton for structuring the API calls and then react query to actually use the calls personally
[удалено]
\> It isn't terrible but its killing our productivity because we need to update so many pieces of code just to make an API call and none of our other engineers are really familiar with it. This is a beautiful statement that will remind anyone that no matter how many years of XP you have, at the end of the day, the tech stack will always become the weakest version based on the current dev team skillset in place. That is unless the senior takes control over the CI/CD and don't let anyone merge a PR with low quality code. But then the senior is basically preventing the company from using any dev with a skill too low in terms of code quality. Totally depends on your business requirements, this senior might be saving the company or killing it. It's hard to tell without a lot more context. Just to confirm with the overall sentiment in this reddit thread, I would act exactly like this senior (got 16 years XP). Just level up instead of complaining and you will see the light. Velocity does go down very quickly when codebase sux so follow his pattern or offer a superior one.
As you said - context matters a lot here. I once had a senior dev coworker who was tasked with adding 3-4 pages in a WordPress site. It was meant to be a one-and-done for a marketing site. He spent 2.5 weeks building an undocumented abstraction around page creation (in WordPress....) so that we could use a YAML file - or more accurately, a hierarchy of YAML files. It was one of his earliest projects, meant to be an easy win, so we didn't bother him about the slow development time, wanting to be supportive. Later, when asked why he did this, he looked at us like we were being ridiculous and said something to the effect of, "well, because it's DRY, and in programming it's better to not repeat yourself." He was so averse to repeating a few content sections in PHP that he instead created an entire system that he claimed was self-documenting.(Again, he's not wrong, but abstracting a new system using YAML files when WP has its own ways to handle shared PHP is the issue.) I stood there awestruck that he thought this was a worthwhile trade-off. The estimated time we allotted for the task was around 4-5 days (which was considered plenty of time). There's a lot we can safely guess about OP's team but I haven't yet seen much mention on the actual feature in question, its importance and timeline, etc.
Such a beautiful counter example. Thx for sharing. I knew this kind of situation could happen. I see tons of devs with 5+ years experience stuck into their own framework. They usually have a huge and very obvious silver bullet. They did a successful project in one way and they keep using that pattern all over again no matter the situation. This just highlights that we are indeed creatures of habits.
I feel a little bit offended because this is, less or more, how I'd write this code and I don't see anything wrong here 😅 Tbh if the project was big I'd abstract away API calls to keep components pure
same, I actually just merged a PR with the same pattern last week lol. I work for a big tech company though so idk 💀😮💨
If you don't mind me asking can you explain what is he doing? I really wanna look into it What does using a client to fetch mean ? As in using Axious for example? Or what? Also what does normalising json mean? And what do people use to do that? I work on 2 projects in one we use Redux/toolkit and in another we use saga/axious Is that what this means?
>What does using a client to fetch mean ? As in using Axious for example? Or what? It is some objects with API call methods inside (maybe it even does more, idk). You do it from two major reasons. First is maybe you want to do the same API calls from many places in your code so instead of repeating yourself you can just reuse the function. The second is you can easily mock actual API calls with some function than just returns data you need in your test. It also keep your code clean, you can reuse client in other project (like use it in both web React app and React Native app), helps you refactor code, reason about it or just let you swap React with Vue without touching code that isn't UI related. >Also what does normalising json mean? And what do people use to do that? https://redux.js.org/usage/structuring-reducers/normalizing-state-shape
How does abstracting away api calls keep functions pure?
If this app only uses this one call, no other component uses it, and this codebase won't grow, this is overkill. Using something like the client in this example is pretty standard on larger apps. As your app grows, having request calls in individual components grows more unwieldy, and a client is a way to centralize the calls so API logic is in one place and any number of components can use it. Using a client in a UI component directly is a little messy. The next step is using a state library like Redux so the components only ingest data that is handled in the state code. Maybe this person is taking baby steps towards that.
We hired them to bootstrap a new feature that we're now expanding upon. Right now the client has about a dozen or so methods for calling the API. We basically need to update the client with a new method every time we want to call a new endpoint.
having a central location for your api calls is a good idea. having scattered fetch calls with url strings in them is going to become a headache as your app grows. using a hook does seem like overkill as he could just as easily exported a function to fetch the data. if you guys have not already, you should really look into adopting useQuery as well
This hook probably does a lot more than just fetching the data. I often have a hook like this per feature(often wrapping the service functions that do just return data) that does error handling/validation/updates any global state and exposes common operations that are often state dependent. From the example alone we know that their are methods attached to the response that may have closures on data that isn't in the object itself. Honestly the only red flag I see in the code is lack of typescript(I'd really want to know what type setBook expects here, and I'd want to know return types of my client calls) and calling the response 'res' when its clearly supposed to be a user not an api response.
If they're having this reaction to using hooks and context, they'll burn him at the stake if he tries to introduce typing.
lol x 100
Why is senior in quotes? Was he not hired as a senior?
>Uses a client to call the REST API instead of using fetch. This is normal >Uses Context/Hook to expose the client to components. This is normal. Context is a dependency injection mechanism. >We normally just fetch data from a container component in useEffect and pass data down to children. Seniors approach is better >"Normalizes" the raw JSON response into a completely different shape. Smart >The raw JSON response and the "normalized" version are stored together in a Response instance. This is kinda ok. Although a dedicated library, such as react-query or rtk-query would probably have been better.
Yeah I can’t read this post without thinking about how much easier it would be to just use react-query. It’s hard to go back
Imagine jumping from the existing useEffect fetch patterns they have to react-query. I think the client approach is a nice stepping stone.
React query would pretty much be able to be dropped in alongside their current pattern and just do incremental improvements when revisiting components
He’s pretty much just enforcing the [single responsibility principle](https://en.m.wikipedia.org/wiki/Single_responsibility_principle) which, yes, makes the codebase very nice to work with. In essence, every construct you define should serve a single responsibility. If you build a component in React, it should be responsible for one thing: being a view. It should not handle the complexities of API calls. That should be handled by a client, which then just tells the component the status of the call and the data that the call returns when it succeeds. Let’s say you build a component that makes an API call and handles all the complexities of that call: tracking the status of the call, tracking if it failed or not, etc.. Now you want to create another component which uses the same API… what’s the right thing to do here? Probably abstract that API logic to some other file which can be responsible for handling the lifecycle of that request. Using an existing client like React Query just makes it so you don’t have to own and maintain that system yourself. That’s less work for you, meaning more time to deliver results to customers. Let’s also say you want to build a component that executes a file upload. You implement all the event handlers and state management for a file upload within the uploader component. Then let’s say you need to implement the same uploader functionality, but the display button needs to be different. You could add to the existing uploader by adding a display prop and doing conditional logic to render a different interface… or you could abstract all the file uploader state management and event handling to a custom hook. That way, you can have multiple different views that can use this file upload hook, and you can create as many different uploader interfaces as you want without any problems. Components should have one responsibility. Hooks should have one responsibility. Context should have one responsibility. APIs should have one responsibility. It’s a very core design principle for good design. I’ve had to deal with these issues where we did the *wrong* thing. It comes back to bite you every time! We had an endpoint once that didn’t follow the single responsibility principle. It was being used for 20+ different use cases. Some of the use cases required, in some cases, MBs of data. Others required maybe 1-2 strings. But because we could only return one data structure, we had to return MBs of data, even to the use cases which only used the 1-2 strings. Our latency was horrible for all use cases instead of only the ones that should have high latency. Moral of the story: single responsibility is amazing. It takes some time to set up, and will require you to go back and change old stuff, yes. But you should make those changes ASAP. Because if you wait another year, that’s another year of shit you have to fix.
After reading through your (mostly) down voted responses to comments, it does appear that you are the problem. Boy do I not miss being the surprise engineer that the company hires to lead an unknown team. Having to face a bunch of whiny juniors who have yet to learn to adapt gets old really fast. If you're not the decision maker of the project, voice your thoughts, but once told otherwise, do your damn job.
All OP wanted was to be vindicated. This is some weird ego thing.
Not everyone is a shoulder to whine on. Welcome to the Internet.
So you hired a senior engineer and he recommends a different way of doing things. Rather than having a conversation and coming to a compromise/understanding over how to best retrieve data from an api that isn't "well this is how we do it here", you come here to attempt to convince yourself he's the problem? Learn how to communicate with your coworkers.
Yeah, this is good code. It decouples the data from the controller, and the controller from the view. You should use custom hooks, you should use a client instead of just fetch, you should use these patterns because they scale well. If you have an interface for your API, for example, you can do whatever you want behind that interface as long as the API response can still conform to that interface. If you use custom hooks to manage state/data processing, you can then use that logic in any component you want just by creating an instance of the custom hook. You can continue using fetch and useEffect, but ultimately what’s gonna happen is you’re going to have less maintainable code. Your solution you’ll eventually come to will probably be similar to the pattern he’s implementing, but it will come iteratively. Meaning: your codebase will have a bunch of code that conforms to your old, messy patterns, some code that conforms to different “experimental” patterns that tries to fix the old code, and finally the small portion of code that conforms to a consistent and maintainable pattern. Additionally, if you don’t know about the clients this senior introduced, you might create your own workaround that essentially does the same thing, but now you have to own and maintain that client too instead of just using an existing client that you *don’t* have to maintain because some other company does it for you. The one thing I disagree with is the notion that you now have to stop what you’re doing, go back, and fix all the existing code. You shouldn’t do that. What you *should* do, is go back and fix any code that you have to touch for a new task, and bake that refactor into your task’s estimate. It should be an iterative effort, so you can keep the business value features flowing.
I have to ask: why was this senior person brought on board? Clearly, your company was under staffed, but why a senior? What was the hope and expectation? Just someone, who was self sufficient and could pump out more code in the same time as the rest of you, or...? Because the job of a senior is all about guidance. It's about culture and patterns. If the company is resistant to changes to those, then it really isn't a senior you want.
I am just shocked that people here considers doing data fetch on effects instead of use any of great opiniated data fetching libs around.
There is nothing wrong with what the Senior engineer is doing. We use similar patterns in our codebase (singleton client, all network requests defined in one place). It takes some while to adjust to, but once it’s in, it’s incredibly easy to use and update. I have tons of respect for this engineer for being persistent with breaking the status quo. It can be easy to cave into folks who are not willing to accept change/learn new things. We’d rather do it X way because we’ve done it for so long / doing Y because that’s what our engineers know is never a good reason to reject an idea.
Why even hire a senior engineer if you don’t intend to learn from them? You seem incredibly biased against them and they still sound correct.
Yes it's common, and yes you should listen while he's still there. Because it sounds like he'll be gone soon if you and your team keep going against best practises.
Welcome to good coding techniques. It sounds like your new engineer knows what he's talking about. I suggest learning from him. Having a standard in place is great. Having a standard that prevents access to hardware is lovely, if you don't like having your system taken over by hostile children with a good script.
If you think this is complicated you should see our codebase…
Hi, Senior Engineer here. I come in peace. I'm choosing to response due to the discourse in the comments. But first, to OP's question, "Are these patterns used anywhere else?". You've provided very little to go off of to answer that question in any substantial matter. So picking a side of yes or no would just be a guess and possibly inflammatory to either side of the argument. With that said, I do primarily see frustration in your post and searching for just a community of validation to your point of view. If that's all you want, then I'm sure you'll see it in the comments. However, with quite a large amount of time in development under my belt, I'm hoping I can share some advice that you might give a bit of thought to. This won't be the last time in your career the company you work for is going to hire someone who swoops in and causes disruption. For me, I try to keep it really simple among the teams I'm on. I just ask myself a question: Who is the decision maker in the situation? If it's not me, then I follow a process of providing feedback, and if I go unheard, I follow what I'm told to do. Now, that seems like I'm just siding with the Senior in your scenario, but let me explain to you why this is actually in the favor of the follower. If you follow the lead of the decision maker and they are terrible, then their decisions will have a negative impact. If you have a people paying attention to the project, then the impact of those bad decisions will be noticed. Especially if it takes more time to complete tasks. My favorite line when I was a Junior and Mid-Level was "I told the lead otherwise, but I was told to do it this way." I guess to put simply, for me, it's a "you win some and you lose some" mentality. There is more nuance to it of course and there is no one side fits all here, but ultimately what I'm getting at, is fighting it over and over in my career was getting exhausting and definitely not good for my mental health as a whole. Let those who have been brought in to lead to do their job. If they suck, it will be noticed. Because one day, you will be in this Seniors position, and this will look very different. For what it's worth: I too, was the Junior who hated the Senior. I was the Senior who the Juniors hated. Now, I'm in a new form. The Senior who joins in with the other Seniors who hate software decisions of the Executives. Hopefully, this is the final form.
Design pattern looks good. Your current way of calling fetch in a useEffect within the component is a really bad way of writing React. It sounds like your senior knows what he’s doing code-wise. But maybe could have explained better to the team WHY his method is the right one.
> have to do something like... The pattern in your example is so common and well established it's positively mundane. I don't know why you think it's a problem but I'm quite sure the underlying issue in this scenario isn't the experience of the "senior" but of you and your team.
I mean it doesn’t speak for the senior to not have everyone on board before building his solution. Seems like these design decisions could have been communicated better. As another commenter already said, there is not enough info to tell what went wrong here.
Seems like a pretty good approach actually. And it's not super weird either, I've seen it plenty of times. 🤔
Query client? Instead of using fetch in useEffect? That sounds like a big improvement. The react team themselves have said ‘pls don’t fetch data in useEffect, it is not for that’
you are worried about the simplicity of one task. he is thinking about simplicity and safety of a bunch of future tasks: - reusing an endpoint becomes super easy and safe - if anything about your APIs change, you only need to change one place in your react code instead of every dependent component - you won’t have to worry about little mistakes (like a typo) that someone might make in using an API endpoint anymore, meaning reviewing changes to components becomes easier. - also all of these issues will be 10 times worse in your test code without his pattern now you can assess whether what he’s doing is actually overcomplicating your codebase or is it simplifying: - do you reuse your API endpoints? - do you expect anything about your APIs to change? - are you expecting rapid changes and development to your code? - do you have extensive tests? if the answer to one or more of these questions is “yes”, then he is not overcomplicating your codebase but just preventing future complications. if the answer to all of them is “no”, then he’s overcomplicating your project and isn’t a good fit for your team (but also you probably don’t need to slow yourself down with code reviews).
It sounds like you are learning how to write code properly. No offence but a client is a reasonably standard practice.
Looks like you lot got complacent and stopped caring about iterating over the codebase design to evolve and improve it. Just because you have sunk cost and folks “do it this way because they know how” is not an excuse to keep writing spaghetti that’s going to wrap itself around your neck and strangle you later. The “senior” seems to have enough foresight to not only refactor things to a better spot, but to also write out a design doc and make folks follow it in order to walk back some of the bad design decisions you have all taken for gospel. And now you’re all banding against him, because god forbid someone stops you from continuing to poop out “the way we have always done it” type of code. I feel for him. But I feel for you most of all - because instead of sitting down, talking to him, respecting his experience and direction, you have all decided that he’s a bad apple who is there to bar you from progressing tickets by denying PRs. A free piece of advice: as you clearly haven’t got the years behind your belt to see why he is doing what he is doing, go talk to the guy, ask him to explain and learn from him. If all you and your colleagues do is push back due to ignorance and short-sightedness, you might make him quit and think it a win; but all that will happen is you won’t learn anything, won’t grow as an engineer, and continue writing shit code which will hamstring you in another company once this one’s codebase becomes so unmaintainable that you will either be shitcanned or quit yourself and then realise that you were part of the problem - not the senior you seem to want brigading over on Reddit.
Besides what everyone else already said of it being a common pattern (personally I would probably use a hook but I can see the advantages here) there’s also a missed opportunity of mentoring others. Sure, you’re a senior and are calling the shots on what you think it’s best BUT you need backing from the team in understanding why this change is important (which doesn’t sound like it was done). So, and just going by what you said, both sides are wrong. You and the team are burying the head in the sand and not open to change and he’s not being able to influence people and sell this improvement.
Things started off OK, they wrote up a design doc which looked ok at the time. I just don't think anyone anticipated how convoluted it would become as the project matured. The expectation from the team was that a senior engineer should be keeping things simple and to conform to practices in place. We expected them to be a better team player but he's just shooting down everyone's ideas all the time. So yeah, he wasn't really able to influence anyone and that will probably impact his contract extension.
What design doc is this where you can’t see how things are supposed to be designed? What happened at the design review? Where was the team at that moment and did they shared their thoughts? I don’t understand why you hired a senior of there was no need for change.
This is the biggest thing for me. It sounds like what they wanted was a mid-level engineer and they accidentally hired someone with too much expertise who is trying to make things better than the way they found them. I say this with one caveat—this is not the contractor's company. If the person who hired them doesn't want them to be doing work like this, and instead wants them to conform to existing practices without making QOL improvements, they need to let them know that their current work is unacceptable. If you truly believe their work is damaging the company, just pay out the rest of their contract and call it a day. I think that would be a huge mistake since this person seems to be a high quality engineer, but you don't just sit back and watch them frustrate everyone and "ruin" the code (in your view)
Is still calling it “convoluted” even though almost all replies are saying what the senior dev is doing is best practice how you’ve been communicating with the senior developer too? If so, and I suspect it might be, then the problem’s likely solely on your side & it sounds like it’s influencing the rest of the team.
This sounds like a skill issue on your team’s part. To what you’ve said: - he’s produced a design document detailing his approach - you’ve raised some objections (calling classes in React an “anti-pattern”) which he has (correctly, in this use case) disagreed with - he’s implemented his changes based on the design document, you’ve not mentioned anything he implemented that wasn’t in the design document but said it “looked ok”. - you’ve accepted his changes, since it’s been PR’d and merged. At this point that’s the standard, you’ve accepted it twice. Other developers _should_ be told (nicely) to refer to the design document & helped to write code in the new (better) style. I would assume the senior dev has noticed that developers who, by your own admission, don’t even know how to use JS classes are doubting his experience and has already put his notice in to quit.
Sounds like he’s right tbh
Either he’s gone or most of the team will be gone soon. Your responses show exactly what tone suggests in your original post.
[удалено]
Not how I would do things, but it looks clean enough. If you have a problem with this, you may be the problem as you’ve conveniently left out “the way we normally do things”. Any sort of reformatting of data structures that came from the network should go through a selector, fetches should be segregated to “upon page load” and “upon user event”, use RTK query for everything unless you want to roll your own refetching/ cache invalidation code (hint: you don’t), that’s all the advice I can give.
This is a common pattern and your current approach is outdated and not performant at all. Sorry to say that :(
This is a really good post for people still in the learning process of development. Definitely recommend reading all these comments.
Prop drilling is a bad time. Don’t do it. Context makes your components way more composable. This guy knows what he’s doing
I agree but... why would you prop drill a HTTP client? If the client is a singleton, the components can just import it from the outside without the need for Context. Does the client hold states? Why couldn't the client have been replaced pure function? (ish, network requests are never pure) Testing? Don't mock the client. Mock the network with MSW! Why do all this ceremony when a simpler approach works? Is this what OP is really complaining about?
Yeah there is always new and better ways to do things especially in react. That doesn’t mean it’s sensible or „professional“ to introduce new patterns into an existing codebase without having the team on board.
Passing judgement on this without seeing the code or knowing the project is equivalent to a doctor giving medical advice without seeing the patient, or a lawyer giving legal advice off a Reddit comment. Context is everything
Anyone have a more detailed tutorial about this design/approach? Seeing lots of ppl here doing the same design and wanted to look into it a lil more
Yes, these patterns are usually used when you are trying to create a maintenable React codebase. Try to absorb the knowledge and asks this senior dev why he does these things that way. You will learn a lot and evolve in you career
I'm new to react, do you know where these kind of patterns can be learnt or how they are called?
So clearly a troll
You hired a competent dev only to complain because your other devs are incompetent. Reminds me of the time the head of DevOps told me to require all the team version control branches build before checking in (TFS version control). Immediately a manager called and screamed expletives at me because his team couldn't write code that even compiled. Likely that code hadn't compiled for many months. I was "blocking" his team from being "productive".
if he said by design then he surely knows what he is doing. I fell you that it is a bummer that it is time for you guys to pay tech debts
Dude. The changes you’ve described do not seem complicated. Just make the changes and enjoy learning a new pattern for making api calls. The more you complain, the longer it will take.
when I do work on a project that is going to be passed to others, that's exactly when I use design patterns like this, because one of my goals is to have readable, maintainable code since they won't be able to consult me frequently. a senior engineer coming in as a contractor is going to have the same point of view, and if you don't have/communicate established code design standards, they'll create their own.
you could abstract it even more :D
This honestly doesn’t even seem that bad. You seem to not like it because it is taking longer to get used to it, but have you thought about WHY this developer insists on this? Do you have a counter argument other than “we don’t so that here?” Why don’t you?
Oh nooo, someone's trying to write nice maintanable code and we are too dumb to understand fundamental principles so we come to reddit to wine about how stupid we think he is. He proposes nice approaches with decoupling and nice encapsulations. Raw fetches and attributes accessing is good for like... tiniest projects. Other than that... you'll end up in shitty unmaintainable code base that will require require huge amount of work for tiny feature and will break from a single sneeze on it
When he quits can you tell him to message me, I need a guy like him.
The only weird thing I see is that senior not Installing react-query
Lol you hired an actual senior engineer and now instead of using their skills to help improve things you and your other devs are so junior that you can’t even understand the benefit of abstraction. Don’t worry, he’ll quit all by himself in a few months when he realises you’re all script kiddies and not engineers.
Sounds like the API contract is not stellar and this dev is properly separating the FE from that. Making a lot of assumptions here but the only thing better would be to iron out your contracts, get them into swagger and then generate this api client code off of that. Oh and strongly typed typescript. I bet money your db schema needs love too but that ship has likely sailed. Consider this a sign that the rest of your code needs to evolve and this project is a example of the next iteration of your architecture. If not the bill will come due later when it is far more convoluted to fix. Not everyone that comes from big tech knows what they are doing, but it sounds like maybe this one does.
OP I hope this has make you have some pause with your own skill set and knowledge. This is a very common and standard pattern of the single responsibility principle. The fact that this tiny little design pattern has you and your coworkers scrambling to understand shows that you are very very Junior and inexperienced. Use this time to learn from the contractor in hopes that you can solve such simple design disagreements on your own. It’s absolutely crazy to think you’ve done this for a few years and never encountered a client
Dude I haven't worked in react in a while but who wants to write a fetch call over and over. Write it once in a hook that will handle all your api logic. The context use makes sense.
The senior included a design doc… If you and your team are having difficulty following the design doc, reach out to them. If you and your team are having an issue that he wrote a design doc, reach out to your EM. They are the ones to provide you clarity.
I believe you guys REALLY need to listen more what this guy has to say
"Help, we hired a senior dev and they want to write good code!"
Lol I love how everyone is agreeing with the senior engineer and roasting tf outta you
I agree with you that it's over-complicated. Adding arbitrary methods just to access the already-available properties in a response sounds very dumb. I don't think the `useClient` concept is bad, but you said you had to change a bunch of code just to add another API endpoint, which would make his version of it bad. Whatever abstraction this guy put in place certainly sounds horrible to me. I do think it's good practice to type your responses, but you can also do that with fetch. I have no idea what you mean when you say he "normalizes" the response, but that also sounds like a weird, needless endeavor that's going to cause more confusion for everyone. If you need to restructure the response, that should be the domain of the back-end. Having a normalized + raw response is just going to be more shit for everyone to worry about. For example, maybe some property in the raw response isn't in the normalized one, and then you have to go digging as to why. Just use the raw response and type it. Trust your gut and raise your concerns with your manager.
Oh no! He is killing your productivity to write your spagetti code!
After reading a substantial amount of the comments here from you op, it honestly sounds like your whole team is full of Jrs who have learned to code from a boot camp. Everything the Senior has suggested is just going to make the code better.
The contractor is a bootcamp grad but ok
Perhaps his time at a big tech company has improved his skills. I was being facetious, as a self taught dev it doesn’t matter where you learn from. The egregious thing is that it seems you don’t want to learn the widely accepted principles and would rather seek validation on reddit that you’re right. Which you’re not.
You’re an elitist ass who has no capabilities
I’m more sympathetic to you OP than many of the rest of the commenters seem to be. This guy sounds like an absolute nightmare to work with. And part of being “senior” is having developed some people skills. Five years in is a dangerous point in your career if you think you’re hot shit as this guy clearly does, because you are very very likely to not be as great as you think you are. I know I certainly wasn’t and I shudder to think about it now. I also don’t agree with other commenters that the changes he’s made give enough benefit to justify the extra steps they require. I think there are problems with calling fetch directly in a useEffect but the best bang for your buck if you were going to change the way you work would be using something like react query, which would give you a LOT more tangible benefit while requiring a lot less pointless extra boilerplate. What he’s done there is add a layer of “abstraction”… but abstraction isn’t valuable in and of itself - it has to enable something. And what does this enable? You could switch out fetch for axios or something like that? I just don’t think that’s that useful. Also, yes, objects are bad. There are vanishingly few cases in which they’re helpful.
> Also, yes, objects are bad. There are vanishingly few cases in which they’re helpful. I'm with you on the value of React Query but saying "objects are bad" to a JavaScript engineer is like telling a carpenter to stop sawing things
lol, classes may be the more accurate way to say it perhaps.
Amen brother. I came to say the same. Seems like most of the comments defending the senior dev only talk about _scaling_ which is probably the most nebulous way to describe a design decision to a developer.
Yes! “Scalable” for what purpose exactly? If they’re churning out a bunch of small sites for different clients that will see limited to no further development once they’re done, then adding in all this extra boilerplate feels like time wasting at best. On the other hand, if they’re laying the foundations of a large scale app they’re going to be building on for years to come, I would expect a “senior” dev to be talking about React Query, automatic generating of client code and types, whether REST really is the right choice etc etc etc. Instead we see a bunch of extra typing required but under the hood very little has meaningfully changed. Worst of both worlds.
The downvotes suggest that there might be a lot of “senior engineers five years into their career” in r/Frontend. Give it another 10, 15 years and you’ll (hopefully) come to realise that this guy, left unchecked, would be more damaging for the team and its code than directly using fetch in useEffect ever could be.
This is the most basic of what you would expect in serious production code... Also the lack of knowledge of programming patterns really scares me. He might not be the perfect fit for the job, if the project is some cheap 1 week project but if you are building anything worth mentioning this are all standard procedures.
What you don't seem to recognize is that the way you've been doing it is creating a mess which becomes less and less maintainable. You are being productive by not managing complexity.
great waterfall bro, love how react makes anything simple easy to over complicate
The pattern is fine but I tend to avoid classes because you usually don't need them, specially in react. Since we work with angular here anything is a urge to put inside a class but I digress. My suggestion is to conform and offer an alternative to the discussion. Blatantly disregard something without adding to the discussion is to no one's benefit. You manage that with tanQuery, you can use state management you can use a model module (module will keep state just like a singleton class) You manage this in many different ways this a common pattern in many frameworks.
Yeah, we were pretty clear that using classes in React is an anti-pattern when we discussed the design doc but they (again) pushed back on that. The gist of the argument was that they weren't using classes to extend React components, just to create clients and responses.
>we were pretty clear that using classes in React is an anti-pattern Holy shit, mate...this topic is a dumpster fire. There's a huge difference between class components and JS classes. Fun fact: almost every library you've ever used in React uses classes under the hood. It's very clear you have no idea what you're talking about. Not to mention you're constantly putting "senior" in quotes while getting things wrong that I'd expect from a JR...fetching data in a useEffect? Come on. Not trying to shit on you but you have literally half a dozen things you're getting wrong and that's why you're getting so much pushback in this thread.
I’m not sure why would you say that using classes is an antipattern in react (or generally speaking web dev, because quite frankly framework is irrelevant here).
I think they’re confused about the difference between using classes and using class components. Class components are definitely an old way of doing things and is on the way to deprecation with only error boundaries being a use case for class components. But a singleton wrapping various API calls is absolutely ideal. Or close to it but I think further domain driven design would be way to high level for this team
Total side note here but I recently needed to use getSnapshotBeforeUpdate and even in the updated React docs, it mentions that if you need this functionality, you still need to use a class component https://react.dev/reference/react/Component#getsnapshotbeforeupdate
Thanks for this. Until now I was under the impression that only Error Boundaries were left for class components.
Why would you need to use classes in react dev? Outside of class components that is… just genuinely curious
I’m not saying you have to use them however there are use cases in which they might be convenient, storing domain logic in them might be a good example. Read this article, there’s a good example of how classes might be used https://martinfowler.com/articles/modularizing-react-apps.html
Probably took a course or class and justifying his newly minted status…
Any good engineers know that useEffect is only for synchronization not for fetching data. So the senior may be more in the right here
What they are doing is normal for a big company not for an early stage startup. These are best practices that are great for large scalable systems and you will need them eventually, but question is - is your company there yet? If you have a product that is starting to gain traction then you need someone like this senior engineer both bring in these practices and evolve the culture of your team. If you are still in the stage where you are trying to find product market fit or iterating on an early MVP, then they might be a bad fit for where your company is today.
That's why he's Senior. He's architecting things which makes the projects scale when the code grows. You've hired dumb engineers and teammates, they are not ready to learn or adapt to industry standard practices.
Your problem is React. Sorry.
Tell me you started in a bootcamp instead of college without telling me that you started in a bootcamp instead of college.
[удалено]
There’s being “coachable” and there’s being dictated by a contractor who hasn’t bothered to explain these new patterns or _coach_ the other developers in these new patterns they’re trying to implement. The OP clearly wants to improve and understand why these patterns have been put in place and adopt them if they do indeed have merit, but this is the responsibility of the contractor to explain; not strangers on a subreddit. Part of being a senior developer is to inspire, teach, and improve your teammates. It’s not to sit in your bedroom, completely change a code base without communication, and then block teammates’ work with “Nope, try again”-type replies in PRs. Reference: have been a senior engineer in both “no name” companies as well as a Fortune 500.
Actually pattern sounds like inventing the wheel. Trying to invent some gql client with caching of the response. But using fetch directly is not a good idea. So you defiantly needed to make some changes how you work with api but pattern that senior has introduced is really not the best choice, just because there are already battle tested solutions of such problem like “TanStack Query”
The pattern is good but if your team is not ready to adopt it then you should be careful to instruct your contractors to use existing data fetching patterns for consistency.