Refactor/design system #481
Conversation
|
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 😅 |
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! 🙌 |
helloanil
left a comment
helloanil
left a comment
There was a problem hiding this comment.
Hey @Paul7Peterson, solid work here. There are some key take-aways we would like to discuss for this PR:
- Using FC for React Components is discouraged, even by the React team. (See here and also see here). We can either use JSX.Element, or let TS infer the type itself, which works very well.
- Regarding refactoring of TpJobseeker to TpJobSeeker, there are two things:
- Jobseeker can be a one word in English, not Job Seeker 😅
- This is very dangerous because we have our DB and BE models in Production as TpJobseekerProfile. (See the screenshot below) So changing these would have a breaking impact in our applications. Can we avoid that for now? These will anyway change in a few months when we complete the Salesforce migration.
- We've seen some changes from
undefinedtonull. Can you elaborate on this one? What's the end goal with these changes?
We've spent 1 hour 15 minutes with @ericbolikowski on this PR and we were able to review only 10% of it. If there's still a chance to split this PR into smaller chunks, we'd highly appreciate it and I'm sure it will be way more time-efficient for everyone. An example pattern I can see here is:
- A PR for semi colons
- A PR for TS in Admin Panel
- A PR for replacing certain words
- and so on
Let us know what you think. But again, we're both astonished with the amazing mind and time spent here. Big kudos many times. True champ!
.vscode/settings.json
Outdated
| "cSpell.words": [ | ||
| "Formik", | ||
| "mentee", | ||
| "mentees", | ||
| "redi" | ||
| ] |
There was a problem hiding this comment.
Issues with the spelling of these words was never brought up by other developers so I'm inclined towards thinking that this might be a very specific problem to a developer. Do you think having these kind of config in your local would make more sense?
In contrast, if we want to have something like this in our project, there are three things we can do:
- We should come up with more words in this glossary
- Have a separate config file for this concern
- Document the usage of this extension and benefits in the Readme file
There was a problem hiding this comment.
I use spell checkers to avoid a big amount of issues that are coming from that. In the PR can be seen in some places how I was fixing many typing errors that I found because of this. The configuration of glossaries can be done per user or per workspace, but makes no sense to be done for the first.
The words in the glossary (that must be in the .vscode/settings.json file) are not for describing terms in the project, but for including terms that are not recognized per the English and language-specific database of the extension. So the first bullet makes no sense for this specific case, but can be done in a separated .md file without any problem. And the second bullet it's because it's the config of an extension and must go in this specific file. I would also recommend setting a .vscode/extensions.json with the recommendation.
About the documentation of the extension, you can name the recommended extension and set a link to the source documentation, but the use of this extensión is exactly the same as the auto-corrector in word.
| "TpJobSeekerProfile": { | ||
| "dataSource": "mongodb", | ||
| "public": true | ||
| }, | ||
| "TpJobseekerCv": { | ||
| "TpJobSeekerCv": { | ||
| "dataSource": "mongodb", | ||
| "public": true | ||
| }, |
There was a problem hiding this comment.
We should definitely avoid this. Please see the main comment on the PR 🙏
There was a problem hiding this comment.
Ok, I will undo everything about the Jobseeker rename.
| const styleClasses = useNotificationStyles() | ||
|
|
||
| const Icon = state ? variantIcon[state.variant] : null | ||
| const Icon = variantIcon[state?.variant] || null |
There was a problem hiding this comment.
This makes total sense but it loses a bit of quick readability when we switch to this. Thinking of more junior developers who can contribute to this project, I think keeping it the old way has more value. What do you think?
There was a problem hiding this comment.
The one on top misses the case of not having a variantIcon with that variant name, returning undefined instead of null, and leading to 3 possible return types.
A junior benefits from being in contact with code that is not made for dummies and that enforces good practices. That's what my experience as Tech Lead and Senior has been taught me. on top you have three elements to analyze; down just two, and one is a fallback, so I can focus on just one element.
| Placeholder, | ||
| } from '@talent-connect/shared-atomic-design-components' | ||
| import { RedProfile } from '@talent-connect/shared-types' | ||
| import { mapStateToProps } from '../../helpers'; |
There was a problem hiding this comment.
Again a very cool idea here, I really liked the helper. Can we just change the name again to make it easier for more junior developers to understand. This might create confusion as it's exported from react-redux with exactly the same name. So if we use a more specific name for the domain, would be easily understood that it's a custom helper. What's your take on this?
There was a problem hiding this comment.
Yeah, a better naming should solve your concern, but I'm just using the one that was set.
| {userType === 'public-sign-up-mentor-pending-review' && ( | ||
| <> | ||
| {' '} | ||
| {' '} {/* TODO: what is this? */} |
There was a problem hiding this comment.
It's just a whitespace to make sure it's not removed by React and we don't have a funky text 😅
There was a problem hiding this comment.
Ufff... These kinds of things are hard, man... HTML has elements for these purposes...
| {currentUserIsMentor && application.status === 'applied' && ( | ||
| <> | ||
| <ConfirmMentorship | ||
| match={application} | ||
| hasReachedMenteeLimit={hasReachedMenteeLimit} | ||
| /> | ||
| <DeclineMentorshipButton match={application} /> | ||
| </> | ||
| ) : null} | ||
| )} |
There was a problem hiding this comment.
It's just a matter of preference most of the time but there are again certain conventions we'd like to follow in the project, according to here. I'm sure it's gonna work the same but let's avoid this kind of a refactor, if it's not gonna bring too much value.
If you have any other ideas, or a strong opinion against the ternary method, can you share with us?
There was a problem hiding this comment.
The shortcut is "faster" and less verbose than a ternary. And considering the size of the components that we are building (that would be another topic), being more concise would be always better.
I understand why ternary is suggested to be used in this "Tao", but in the end, it's an opinion of a developer and the only given argument is a bad return that, in case of being done with a ternary, leads to a 50% of chance to match the expected behavior. So for me, the ternary is a trap; is a way of hiding a possible error inside a bad understood "poka-yoke". And considering that unit testing has not been a priority for the design of the components, it's way better to have a weird return of a 0 or whatever so evident wrong thing to be rendered that can be noticed than a rendered-or-not component based on a not-so-transparent behavior. A ternary is ticking yourself in solitaire.
| export function createComponentForm<C extends object> () { | ||
| return { | ||
| /** ... */ | ||
| validation<T extends object> (formSchema: YupConstructor<T>) { | ||
| const validationSchema = Yup.object(formSchema(Yup)); | ||
| type FormType = ValidationObjectType<typeof validationSchema>; | ||
|
|
||
| return { | ||
| /** Definition of the initial values using the context of the component */ | ||
| initialValues: (initialValues: (context: C) => FormType) => ({ | ||
| /** ... */ | ||
| formikConfig: (config?: FormikModifiedConfig<FormType>) => ({ | ||
| /** ... */ | ||
| onSubmit: onSubmitFactory(validationSchema, initialValues, config) | ||
| }), | ||
| /** ... */ | ||
| onSubmit: onSubmitFactory(validationSchema, initialValues) | ||
| }) | ||
| }; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Another very cool idea here, we definitely needed an abstraction for the sake of consistency of the forms in the project. Very well done 💯
However, I have some concerns here:
- Should createComponentForm utility be in a file named
yup.ts? Yup is a tool to use for validation but this function does way more than accepting validation. We might need to move it somewhere else. - Also, with this kind of an API (chaining functions to each other), we lose the sake of configuration being order-ignorant. I mean, when anybody uses this function, they're enforced to use the functions to create a form in the exact order you set. Also, for the forms that don't require any validation or initial values, they will need to be sent in the right order with an empty object, just to make sure onSubmit works. I think this problem can be avoided with a good documentation but again, this can be a very mind and time consuming process for a junior developer, which we have time to time, contributing to these projects. Do you have any ideas how we can tackle this challenge? I'm really keen on discussing this further and make it work because this idea is very very cool!
There was a problem hiding this comment.
I would rename the file to form-helper.ts or so, yeah. The folding of the project it's a topic that I still don't want to open until this PR is ready, but there's a big room for improvement when it comes to organizing the whole repo. Some good practices were already applied in the design system library as you can notice.
The order of the functions serves a strong typing purpose that eliminates the inconsistencies between three elements: formik, yup, and the initial and referenced values. I found situations in many components where the typing casting was covering a bad definition of the variables, leading to unexpected behaviors with undefined or even worse scenarios with an absolute mismatch between the use of variables and configurations. This specific order and currying solve that:
- A friendly and unified API for the creation of the logic of forms.
- The possibility of unit testing the form behavior independently of the UI implementation.
- Eliminates the continuous imports done in all components, leading to clearer and smaller bundles.
- Allows using naming conventions of files for separation of concerns and easier PR reviews in the future.
- Keeps the react components cleaner and allows a lot of reusability that is still not implemented.
- The junior developer doesn't require to fully understand the logic of the
formikandyuplibraries, creating an abstraction that makes it way more comprehensible and easy to use. - The
validation()step can be used only for type definition, lettingyupdo the parsing for us without any further config, leading to an empty object return ininitialValues()but still having, in the end, a returnformikobject that you can use with the design components as they are (while we decoupleformikfrom them, with is a terrible thing to see and that, in my opinion, makes that something like a Dropdown select has an API 10 times more complicated than this factory and with zero documentation). Showing juniors how to solve complicated problems with easy solutions like this must be also a priority in a learning path. - I still have to properly document this in the JSDocs, but since everything is pretty straightforward; it makes the logic of the form way more comprehensive with some embed docs than the wild and wrong compositions that I've seen.
|
Hey @helloanil, to answer the top-level topics you proposed.
About splitting the PR:
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 id | Secret | Commit | Filename | |
|---|---|---|---|---|
| 753993 | Generic High Entropy Secret | b3e7f20 | nx.json | View secret |
| 753993 | Generic High Entropy Secret | ad5445b | nx.json | View secret |
| 753993 | Generic High Entropy Secret | 863d333 | nx.json | View secret |
| 753993 | Generic High Entropy Secret | e7ef109 | 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!
What Github issue does this PR relate to? Insert link.
What should the reviewer know?