connect icon indicating copy to clipboard operation
connect copied to clipboard

Refactor/design system

Open Paul7Peterson opened this issue 4 years ago • 6 comments

What Github issue does this PR relate to? Insert link.

What should the reviewer know?

Paul7Peterson avatar Jan 13 '22 14:01 Paul7Peterson

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 😂

helloanil avatar Jan 13 '22 22:01 helloanil

@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 😅

ericbolikowski avatar Jan 31 '22 14:01 ericbolikowski

@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.

Paul7Peterson avatar Jan 31 '22 16:01 Paul7Peterson

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! 🙌

ericbolikowski avatar Jan 31 '22 16:01 ericbolikowski

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 the FC<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 export is 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 Jobseeker rename, I won't discuss it. Your argument is totally right, so I will undo that.
  • About the null vs undefined; every time that I see a written undefined in the code, it gives me goosebumps. The biggest distinction between both is that undefined is provided by the behavior of JS/TS and null is an explicit absence. No code in vanilla JS returns null, it's always the intention of someone explicitly saying that the value is missing. null indicated 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 // TODO will 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.

Paul7Peterson avatar Feb 06 '22 09:02 Paul7Peterson

⚠️ 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
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. 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


🦉 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!

gitguardian[bot] avatar Apr 27 '23 20:04 gitguardian[bot]