Refactor/design system
What Github issue does this PR relate to? Insert link.
What should the reviewer know?
Hey @Paul7Peterson! As I have mentioned also on Slack (also writing here for better visibility for everyone), let's try to avoid changing the format of all files in the project.
Although I agree with the format rules of your formatter, adding this complexity for this work will only make it impossible to review this PR. Reviewing 409 files would probably take ~2 months for me 😂
@Paul7Peterson, did you use any codemods? I see systemic changes changes, I'm curious how you managed to be so detailed. I hope you didn't read every line of code 😅
@Paul7Peterson, did you use any codemods? I see systemic changes changes, I'm curious how you managed to be so detailed. I hope you didn't read every line of code 😅
I go full vainilla 😅 So yeah, I was doing everything manually and reading everything line by line.
In the end I'm a senior developer, so I know how to do this kind of refactoring and I normally follow some patterns in the search to find the common issues.
I go full vainilla 😅 So yeah, I was doing everything manually and reading everything line by line.
In the end I'm a senior developer, so I know how to do this kind of refactoring and I normally follow some patterns in the search to find the common issues.
Kudos! 🙌
Hey @helloanil, to answer the top-level topics you proposed.
- You're absolutely right with the
FC<Props>approach and I recently encounter the issues described in the link you set. Years ago, when I learned React, was the easiest way for typing components, but TypeScript is way better nowadays. I made that change to be able to use TypeScript to catch issues, but I will remove theFC<Props>approach and change it to a direct typed function. I'll make sure that everything works as expected and preserve the changes I made for type inferring. - A small comment based on the previous point; I've seen that you use the default export for the components. Is there a specific reason for that? My concern and the reason why I personally void the
default exportis that you can and have to use your own name on the import of the component and even sometimes; destroying the documentation chain on the way. - About the
Jobseekerrename, I won't discuss it. Your argument is totally right, so I will undo that. - About the
nullvsundefined; every time that I see a writtenundefinedin the code, it gives me goosebumps. The biggest distinction between both is thatundefinedis provided by the behavior of JS/TS andnullis an explicit absence. No code in vanilla JS returnsnull, it's always the intention of someone explicitly saying that the value is missing.nullindicated that there is intentionally no value (like in compiled languages), and not because of some wrong access to the memory position.
About splitting the PR:
- For the semi-colons in the JS files, yeah, I can remove them.
- The change for the admin panel to TS is to catch problems, but yeah, it could be also part of another PR.
- The words I do it when I see the issue, that way I don't forget about it. Setting a
// TODOwill increase also the PR without solving the issue, and we are even discussing semi-colons.
The changes done are all derived from the changes in the design system components. And maybe some refactoring could have been done in another PR, but takes me less time if I do it when I see it and for me, it's just natural. There are way more things that I already commented out that are coming out from seeing things, but they will imply way more changes and I consider that they can be done after this PR as we already discussed. But correcting small details that I see and typing most of the things the right way is helping me a lot to make everything work and to catch hidden issues. I know it looks like I'm touching things that are not related to the purpose of this PR, but they are when you follow the trace of bad practices and errors' covering. That's why I've invested that much time; so I can trust TypeScript and my own testing for the changes in absence of unit testing.
⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.
Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.
🔎 Detected hardcoded secrets in your pull request
| GitGuardian id | Secret | Commit | Filename | |
|---|---|---|---|---|
| 753993 | Generic High Entropy Secret | b3e7f20aa522aa9a7e750db89dc32b1519696f3c | nx.json | View secret |
| 753993 | Generic High Entropy Secret | ad5445bbbd5889e34d16211c649435a9a1e4de0f | nx.json | View secret |
| 753993 | Generic High Entropy Secret | 863d3337f39430ff8d4fecfee8f8598aa0b7a728 | nx.json | View secret |
| 753993 | Generic High Entropy Secret | e7ef109e4185b8be19b11688b0277273d47da188 | nx.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!