And the best thing someone reviewing code can do. Just set up ESLint to error bunch of important rules in CI and you will save yourself a lot of time while reviewing code.
You added several new dependencies for the little functionality there was. Not sure if I'd call that a win unless the entire application is refactored (now or in the future) to make optimal use of those libraries. Maybe an idea for future videos like this, do a refactor without additional libraries. Then, take it a step further and use the libraries. As far as refactoring goes, you really just rewrote everything, altering functionality in the process: - toasts - min/max username/password characters requirements (this could have consequences throughout the app as there were no such requirements before). Preventing empty fields could just be done by adding required to the input. Regarding putting the login path in a config file, seems unnecessary to me as this doesn't typically ever change and is usually only used in one place, the login page. I'm sorry if I sound nit-picky, but I've seen people refactor things in this way, introducing problems and bugs where there previously were none because they started changing things in the same way you did. I mostly commented because you said refactoring, refactoring means improving the code without changing functionality and that wasn't the case here. Not sure if you used the term as a mistake, just felt I had to point this out as it might teach people the wrong things.
I completely agree with you. If this is "refactoring", then I assume that the business logic and UI are provided by the designer I saw in this video: 1) Renaming variables and comments (it's funny that the author at the beginning removes comments saying that they are not needed, and then adds it to dispatch saying that here it would be possible to write :D) 2) Renaming the UI text (you can't do that if you assume that the text has already been approved) 3) Breaking the scrolling logic (you can't do that if such a UX is approved by the designer) 4) Complains about the response from the data.access server (I believe that this is an approved contact from BE. You can't just take it and rename it if you are FE, because it will just break if you are not supported by BE) 5) adding new dependencies to the project 6) Rewriting to TS and complained that in JS you can't catch errors of code before launch 7) added validation just like that, well, because author wanted to validate (you can't do that if we believe that validation was not originally intended) If tests were written on this original code, they would all be broken I would call this video "how would I write a form"
Very informative video. Learning from other people's mistakes is a lot less damaging to both our businesses and our ego than having to learn from our own. More of these please!
There's a potential bug in this refactor (and the original): The onSuccess runs asynchronously and uses getValues(). The user might modify or delete the input field value if the login handler takes a long time to run, therefore the user will be logged in with the wrong username. Instead you want to get the username from the onSuccess parameters, which provide you access to the parameters that were used to invoke the mutation. This way the username will always be correct!
Someone else also pointed out - Maybe an idea for future videos like this, do a refactor without additional libraries. Then, take it a step further and use the libraries. I second this I think the best advice for that beginner would've been to show them form validation & good code structure from scratch without libraries, then use these libraries show them how they make things easier and what are the tradeoffs (controlled vs uncontrolled forms). Then they could try themselves to take it a step even further & create a Form store or HOCs and do everything through a custom component. Another very helpful comment pointed out that in an actual project the UI/UX, data from backend would be already decided and approved, and if this is how they decided it your job is to make it clean and maintainable without changing those things, something to keep it mind which is why doing a few things from scratch first helps a lot in learning
I didn't really know the importance of react-hook-form and zod but now that I saw the combination of those two and how amazing it is I'm going to be start using. Also I used to use react-toastify which is good but the toast from react-hot-toast is much cleaner (imo) so thanks for this really amazing educational video.
I believe that merely learning the syntax without gaining a deep understanding of how things work is a detrimental practice for beginners. Your advice is beneficial for experienced programmers; however, I think that writing the entire code and using as few tools and libraries as possible provides beginners with more knowledge. Simply copying and pasting boilerplate code without thinking may hinder their learning process.
Good point. It's always a split between the complexity of the video and the detail of implementation. This is what I would personally do in my own projects, so I share it here. Of course, you can build these yourself. But using these battle-tested and well-documented tools makes much more sense imo. If a video gets too detailed, most people will not listen because it takes too much focus. Building these from scratch once, not for productivity reasons, but simply for learning as you said, is a great idea.
Thank you very much. Here’s something I’m sure the whole community would love can we stimulate from beginning to end a production ready next js apllication that is feature rich, I’m sure I’m not the only one that gets stuck when asked questions about production.
I absolutely demand you to continue this type of videos. Next time I will definitely submit my project and let you check it out! Also, try not to say "Why the hell...?" or "Why would it be...?", because it is a bit rude.
@@joshtriedcoding To be honest I like that the review is more natural, no need to try and be politically correct about everything. Yeah don't just shit on everything, but saying "what the hell is this" is totally valid and a natural thing to say. You can always say "I mean, yeah I used to do the same thing" to tone it down, but no need to sugar coat everything all the time :D
I wouldn't say that adding typescript and form library is a fix of mistakes. Those are tools, the same as styled-components. By shoving in ts you didn't improve code quality, you switch language practically for almost no reason, because for safety you could just have jsdoc comments.
Oh, and about form lib: you don't usually need it if you just have 1 form. Yes, it's good, declarative, many usable features, but for one form you can keep it simple.
Josh can you please make a video that tells everything you should know to make a secure earning through freelancing. I am really starting to get demotivated because of no projects. Please tell which skills I should know in 2023 to be a top web developer.
You need to do more of these videos! This was amazing. Never even thought about combining react-hook-form and usemutation together! You should do a video going into more detail on that
Great video! I'm relatively new to react and this peer-review style video is EXTREMELY helpful. Would love to see more videos like this :)) But for a question - In a project that is using redux, what considerations would you account for while choosing between rtk query and react query? Are there any major functionalities/benefits/performance gains for one over the other?
@@joshtriedcoding hah I still don't get why, it was a honest question, because I never had a type related error in 6 years, so I'm trying to understand what am I missing each time I see it mentioned as a game changer
@@johndowland4623 bro had an awful answer for you here fr. I feel like if anything, TS has proved to be a lot more painful than JS in some instances for me. Especially when I get some warning that looks horribly scary at first glance like "Type '(e: React.FormEvent) => Promise' is not assignable to type 'FormEventHandler'. Types of parameters 'e' and 'event' are incompatible." I think in the long run though, since TS will ultimately get transpiled into JS, it's more so a means for you to be confident that you understand the data being thrown around in the code, avoid type mismatches, and avoid unwanted type coercion while actively coding, especially on larger projects where it's harder to be sure what will happen when you change something in 1 file.
Your password validation gives an error message for 8 characters, but the code only specifies "min(3, ..)", probably a copy & paste error from the username validation above. This proves that the old code was better 🤓
This was great and it's what I do, but I admit I feel it does make things more complicated, i.e your approach (and mine) means you have to know how to use react-hook-form in-depth...I wouldn't consider it simple by any stretch, like conditional validation, triggering validation, watching inputs, using it with Selects, date/time etc. There you go, a video idea, react-hook-form, but more complicated examples.
Good tips. Proper naming of functions/variables may seem insignificant but is so important. Makes it so much easier to understand code by colleagues, AI assistants and yourself in the future. I do have a complaint on package usage. For a login form, why would you bloat your JS with zod, hookform resolver, axios, react query, when without them you can have everything they're offering with the same amount of code? RHF could by itself handle the type-safety with validations, as well as provide a flag (isSubmitting) for monitoring the mutation (which could just be a native fetch).
For example, to me personally it's just nicer to work with RHF + yup/zod and axios. And you mention "for a login form...", but let's be real in an app you will never have just a single form, you will have many so why downgrade your experience with some homemade solution when you could use a battle tested library. I do like defining schemas in yup and then use them for react-hook-form validation because it's nicer to read and write (subjective opinion) and I can also generate a TS type from that schema and have my form data typed easily that way and I can use that type anywhere I want. And the best thing about that combo is that even junior developers can write really good forms because they don't have to go into some homemade solution that might be hard to understand and is not documented. Also, I prefer axios because of interceptors and the ability to show upload progress (which native fetch cannot do). I mean, in theory you could always write everything from scratch, but there is no point in doing that. I do agree with "isSubmitting" and it's something I've seen people ignore or not know about and do their custom "isFormSubmitting" useState all the time.
At 2:42 you are saying that the window.scrollTo is a bad idea, but you don't explain why this piece of code is here for. I consider this piece of code as valid but surely it is not in the right place. When dealing with mobile devices, the adress bar of the browser can be really annoying if you are making a fullscreen page. Content at the bottom will be hidden if using 100vh as on mobile the viewport is sized without substracting the address bar height. You have solutions to this, for example : - Use `height: 100vh` on the body then use window.scrollTo to get rid of the address bar. - Use `height: 100%` everywhere but you keep the address bar all the time (This one is mostly used everywhere) However since end of May 2023 a new CSS unit is supported by latest versions of browsers on mobile (also on PC obv) : `dvh` solving this problem. BUT. As it is not supported by older versions of browsers, it's not recommended to use it for now.
I've been using formik and Yup for validation. It's been great!! And I totally feel you about not being able to use Javascript. I'm a computer science student rn, and a friend of mine asked me to do some freelance development for him, and I straight up told him ki no, i can't do it in javascript. I'll use typescript. Take it or leave it.
Cheers, Josh! I learned at lot here and would appreciate similar videos in the future, perhaps one highlighting Typescript and / or security improvements
Limiting passwords to 20 characters is a bad design aswell. Password fields should accept at least 64 characters and all types of ASCII chars to avoid conflicts with generated secure passwords from password managers like KeePass
I don’t know, man. This component is still doing and knowing about too much. Classic react spaghetti. Why react-hook-form and zod? There are literally two inputs; browsers APIs can handle it without wasting kilobytes. Instead of zod, you could write your own validation functions they are pretty straightforward. Is react-query used only for the isLoading property? I mean, it’s not using the library’s feature - cache. It’s not updating its entities, nothing, just firing the success callback to update the redux store. Is good old axios not okay for it? Also, the only way to test this code is by running pretty pricey integration tests. Bottom line, the refactored code is questionable, if not worse. Happy hacking.
Yeah I would definitely use Jotai and RQ instead of Redux. The thought of what a complete ugly convoluted mess of boilerplate Redux is is making my eyebrows twitch.
can you do common practice in styling using mui, my currenct project using wrapper for each component and write classed inside thes. dont using inline style, create another styled component if need for reuse. is this good practice?
So you only have to write the validation once. Outsourced to a separate file, you can use the validation for both server and client and only need to change it in one place when the requirements change.
@@ChristianKolbow Cool, but you can (and should) also outsource your validation and business logic into separated files using React-Hook-Form alone. Can't speak about the server part, but since they are only functions and interfaces I asume it would also work?
@@imsuvasco7922 I'd say for those used in multiple perhaps u want to extract it, for one time use it's really up to u, if u feel like component gets dirty with this big query go ahead and put it in the file next to that
As expected from you, Very precise review brother, just some recommendatios, adding too many external libraries for something that seems like a personal project isn't necessary. It basically adds a lot to your bundle size on top of a heavy library like react, and if you are a beginner and don't feel the need to use Typescript, native HTML form validations works fine.
Yeah, this definitely adds some complexity. This is how I‘d personally do this, and this also goes for any form in the entire app, not just this. Chances are you’re gonna have a bunch, so once you get the hang this approach pays big dividends
One tip, you don’t want to say the minimum let alone maximum length of a username let alone a password. It makes it easier to realize how to brute force it and even know how many iterations it users. Login errors need to be ambiguous: “login failed! Wrong username or password.” You can enforce the fact that they both need to be non-blank but that’s it. Also variable names, I don’t give a hoot. I am so old school that we tracked 16 but addressed and registers for variables. A good programmer can immediately see where what is used - say you did. aT was more than clear to me. And I find that for something that isn’t even OAuth that there are far too many libraries used, it makes it very heavy for something as trivial as two fields, that just need to validate if the values aren’t empty and send it to the server and return the server response or a valid token and send that on for authorization. But that’s my gripe with web and Python devs in general, code is too bulky and too reliant on dozens of libraries for even the simplest things. I would only use an OAuth library library, because OAuth is pain to do and to do efficiently and safely, when you do not know the protocol. So those are my two docents as a former Identity and Access manager for big enterprises. Other than that a nice video!
@@keemkorn Indeed I would likely call it aTkn and bTKn, no way I’ll call it accessToken. Clear enough for anyone who deals with access management. And a good language and AM implementation even has two different types for those that will clear things up even more. My point is that developers these days thing that long descriptive names make software better readable. The thing is that it doesn’t often they gets so long that typos and reading errors occur especially for those that are very similar. Us old guys learned to juggle single or two letter variables because memory was tight. And in assembly we juggled memory address as variables, until we got ab assembler and exen there we used small names because 20 chars or so on a line with indent was about what you had to write an instruction and remark. So I’m fine seeing vars with one or two letters I read the code and occasionally check a type (which editors now just display very conveniently in case or Zig and Rust).
@@CallousCoder damn, you really ARE a callous coder. I hope I never have to work with someone like you in my life. Never work under the assumption that someone who’ll look at your code will have the same amount of knowledge as you, that’s just unfair. accessToken is just fine; something like superSecretAccesTokenUsedForLoggingIn is not. And all that “back in my day, we ate the leather off our boots” junk is so tired. Try to get away from that mindset bro, we aren’t coding in Perl anymore
I'd greatly appreciate the opportunity to look at your modified code as a whole. While I know I can skip around your video, this is rather tedious and I may not even find what I'm looking for. Having the original code repo alone doesn't really help that much. :/
Interesting video, I am unsure as to why you are setting min max messages for a user login. I have never seen that done, if anything the only time I have put a restriction on the username field is if it is to ensure that it is an email address otherwise it just seems redundant. if it was a register page sure why not.
Great video! However, If I was a bambi react developer Im not really sure If I could understand any of this. The fact that this dude's code is in JS and you explain it in TS is a thing. Adding all that extra libraries for two inputs is kinda unnecessary. A simple validate function or a required attribute (for this project) would have been enough. For futures videos like this which I will watch for sure I would suggest going easy mode at first but still like a pro. Then like an "extra" you could take it to another level and write all kind of stuff.
Yeah... I'm really not a fan of this video. What you're saying isn't wrong, but the attitude is just... not fine. Also, if you don't know how to use JavaScript as well as TypeScript, that just shows your inadequacy in understanding how JSDocs work. You can achieve the same IDE assistance by simply using correct type definitions.
Hey Josh, I personally think NextJS is making things unclean day by day, it should not be the case, using that "use client" keyword or "use server" in middle, also we don't have any choice of creating dynamic routes without that folder convention, what you think let us know. TIA
Yeah the "use client" has been criticized for being a bit misleading. After all, your html is still prerendered - that's why seeing the page even works with javascript disabled. It's only hydrated on the client afterwards for the interactivity. Besides that and the caching issue with the app router I really like the new changes
Will not be better to use nullish coalescing operator `??` instead of OR `||` (e.g., in `location.state?.from?.pathname || '/dashboard'`)? Great vid, thank you!
If you use the nullish coalescence, wouldn't you get redirected to the homepage if you came from there? Because an empty string would not be included in there, whereas with the or-operator it would
Clean code is a trigger word on the whole clean architecture probably in the world of C#. I agree with “no politically correct required”, but please avoid disinformation. Naming concisely is hard enough. Also, about passwords, it ultimately comes down to the bit size of the hashing algorithm and nonce, no? Why limit the password length?
In my Team when we used styled-components, we would move it to file ComponentName.styles.tsx And would make in really rare cases , because most of the UI design system should cover
Right now we have completely switched to tailwindcss, and it has been the best leap in development experience for years. It was like switching from Notepad++, or Eclipse to VS Code (almost)
yeah it seems I might have sounded a bit judgemental at some points. That is the exact opposite of what I want to communicate here. This dude did a great job that we can all learn from. There is no wrong code, so don't worry about it while writing. You can always go ahead and refactor later.
Hey, i am video editor, and as a professional i want to say that you are spreading great values but you are lacking in editing. If you have long term goals, and believes in youtube, then i can help you in editing part.
If I embarrassed you, then I apologise. You’ve done an awesome job man. Really appreciate the code you wrote and how much there is to learn from this. You can be very proud of your work.
