Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General - Research structure for front end organization and make write-up of front-end practices #55

Closed
jpwaves opened this issue Apr 17, 2022 · 8 comments
Assignees
Labels
spike Research

Comments

@jpwaves
Copy link

jpwaves commented Apr 17, 2022

Spike Type

Technical

Goal

Improve the organization of front-end components and overall project structure for things related to front-end, and create a wiki or docs page listing out what practices and style we want to use for all our front-end files.

Reason for Spike

To standardize our practices so it's easier for new devs to onboard and all our code follows best practices (to some extent)

Additional notes

may need to look into how we want to also test front-end components too and make a doc on that

@jpwaves
Copy link
Author

jpwaves commented Apr 21, 2022

File Naming Conventions

For .jsx files, I recommend switching to using PascalCase so that we can denote to devs which files have exported components and which don't. This will also help differentiate between which of our files are backend and which are front end (especially since we have both front end and backend files in the same repo). The file name should also match the name of the component that is in the file e.g. CustomerCard is in the CustomerCard.jsx file.

Project Structure

We should separate our current components folder into two separate folders: components and pages. components will contain all of our reusable components; pages will contain the React components that correspond to actual pages (or parts of an actual page) of the dashboard. For our components folder, I think it makes sense to test to keep grouping files by components as we are doing now, but for our pages folder we may need to look into a different approach if nesting becomes too much of a hassle for devs to access deeply nested files. For now though, we can keep grouping files by individual components for both components and pages for now.

General Thoughts

  • We should move all business logic and such out of our components and into classes that will live in services. This should help prevent convoluting our components with too much logic.
  • We should consider converting shared into helpers and putting any helper functions we have into that folder and move them out of our components. Alternatively, we can convert our current utils folder to store both the helpers and also our constants (which will probably our theme color constants and other values that are constant across the dashboard).
  • I don't think having two different package-lock.json files in our repo is good to have. It makes it very confusing, and I honestly have no idea why we need 2.
  • We should separate our hooks and API files into hooks and api respectively, and potentially move them up several levels in our project structure if it makes sense to.
  • May need a common folder if we find that some things are reused extensively throughout our application and moving them there
  • We should create aliases for absolute paths in our config files
  • We should actually build out our babel config and maybe look into adding in a webpack config as well.

https://reboot.studio/blog/folder-structures-to-organize-react-project/
https://www.robinwieruch.de/react-folder-structure/
https://reactjs.org/docs/faq-structure.html
https://www.taniarascia.com/react-architecture-directory-structure/
https://stackoverflow.com/questions/55221433/is-there-an-official-style-guide-or-naming-convention-for-react-based-projects#:~:text=React%20components%20are%20PascalCased%20by,for%20javascript%20projects%20in%20general.
https://www.upbeatcode.com/react/react-naming-conventions/

@RChandler234
Copy link
Contributor

Yeah I totally agree with all of this; I think it'll go a long way towards making the codebase a lot more accessible for new members as well. I would suggest doing it in stages just so we don't confuse people. Imo, the naming convention stuff/ pages and components folder stuff should just happen asap though. I would also appreciate an explanation/justification for having 2 package-lock.json files that seems weird.

@anthonybernardi
Copy link
Collaborator

definitely agree with all of this. components have way too much logic in them and we need to start simplifying some of the pages before they get out of hand. The project restructuring sounds great too.

I have issues with our 'utils' folder in general although I may not understand the reason why we have it the way we do.

@jamescd18
Copy link
Member

For .jsx files, I recommend switching to using PascalCase

This sounds good to me. Just make sure we’re changing the file name and the containing folder name.

so that we can denote to devs which files have exported components and which don't. This will also help differentiate between which of our files are backend and which are front end (especially since we have both front end and backend files in the same repo).

I disagree with this. Have you seen this being done in industry? I’m of the opinion that devs need to learn to read files. How do you tell if something is exported from the file? Read the file contents. How do you tell if something is back-end? It’s in the back-end folder. These are not things I think should be handed to them on a silver platter.

We should separate our current components folder into two separate folders: components and pages.

Sure that sounds good to me.

We should move all business logic and such out of our components

Agreed, where possible. Business logic should primarily live in the back-end, and be presented by the front-end.

into classes that will live in services.

I’m not sure about this. Services are what connects the front-end to the back-end. Business logic should not live in there, unless we have a different understanding of what business logic is. I’m also hesitant to say it should be classes. Our project doesn’t use class-based design patterns, so I’d be curious to see what you think it should be and why you think it would be beneficial.

We should consider converting shared into helpers and putting any helper functions we have into that folder and move them out of our components.

This sounds good and makes sense to me

Alternatively, we can convert our current utils folder to store the helpers

The utils folder is a local module. You can’t be treating it just as another folder. It is intended to be an isolated location for code that explicitly needs to be shared between the front-end and back-end or needs to be shared across endpoints in the back-end. It would be best to keep it limited to that, so as to avoid bloating the bundle size of the utils module.

constants (which will probably our theme color constants and other values that are constant across the dashboard).

Having constants would be a good idea.

I don't think having two different package-lock.json files in our repo is good to have. It makes it very confusing, and I honestly have no idea why we need 2.

Which two are you referring to? If you’re referring to the utils module’s package.json and package-lock.json as the second ones, then they’re required. See previous explanation about the utils module. Every Node.js module needs its own package.json and package-lock.json files.

We should separate our hooks and API files into hooks and api respectively, and potentially move them up several levels in our project structure if it makes sense to.

Sure, yeah, that could work. Though, they are the services, so I’m not sure what you imagine services would be from a front-end perspective if not hooks and api methods.

May need a common folder if we find that some things are reused extensively throughout our application and moving them there

Sure

We should create aliases for absolute paths in our config files

I swear we had a ticket for this because I remember someone trying it before and not succeeding, but I can’t find the ticket so oh well. Definitely a good idea if we can get it to work.

We should actually build out our babel config and maybe look into adding in a webpack config as well.

Sure, but this falls under DevOps, which is something I’ve been saying needs more dev attention the entire past year.

@jpwaves
Copy link
Author

jpwaves commented Apr 22, 2022

I disagree with this. Have you seen this being done in industry? I’m of the opinion that devs need to learn to read files. How do you tell if something is exported from the file? Read the file contents. How do you tell if something is back-end? It’s in the back-end folder. These are not things I think should be handed to them on a silver platter.

I believe I read this from one of the articles I linked. It's partially done at Wellframe in our repo for all our front-end stuff. I agree that devs should learn to read files, and although things definitely shouldn't be gifted to devs on a silver platter, they should still look for ways to make their lives easier and I thought that this was one way of doing that (which I guess is kinda contradictory). Can I take this as you agree with the action of using PascalCase for our file naming convention but disagreed with my reasoning? If so, are you favoring applying this naming convention to all files or just front-end files?

The utils folder is a local module. You can’t be treating it just as another folder. It is intended to be an isolated location for code that explicitly needs to be shared between the front-end and back-end or needs to be shared across endpoints in the back-end. It would be best to keep it limited to that, so as to avoid bloating the bundle size of the utils module.

But why do we have this local module inside our repo? Are we not able to move this up and combine it with our top level package.json? Also, what is the main point of this module being here? You've probably gone over this before, but I forgot. Also, we can just use common as the folder name and stuff both helpers and constants into that since that would still make sense.

I’m not sure about this. Services are what connects the front-end to the back-end. Business logic should not live in there, unless we have a different understanding of what business logic is. I’m also hesitant to say it should be classes. Our project doesn’t use class-based design patterns, so I’d be curious to see what you think it should be and why you think it would be beneficial.

I kinda include everything that's not code related to displaying a view as business logic, which is probably incorrect LOL. and also why we have different understandings of what business logic is. Tbh I mentioned classes because that's what my co-op does for their services. They use class-based services to make API calls and also to house related helper functions (like transformers). I did not completely research this section so I can look more into it, but having it be class-based looks nicer in my opinion than just having a file with a bunch of functions (albeit the class thing is just boiler plate code and it wouldn't take in any constructor params or anything).

@jpwaves
Copy link
Author

jpwaves commented Apr 22, 2022

Sure, yeah, that could work. Though, they are the services, so I’m not sure what you imagine services would be from a front-end perspective if not hooks and api methods.

Mentioned this above, but I would also include helper methods that are closely related (e.g. transformers) as a service as well. Not sure if that's correct though since I have a limited knowledge of what services should be

@jamescd18
Copy link
Member

Can I take this as you agree with the action of using PascalCase for our file naming convention but disagreed with my reasoning?

Yep! That’s accurate.

are you favoring applying this naming convention to all files or just front-end files?

Not sure, tbh. Give it a whirl whatever way you think is best and then let’s go from there.

But why do we have this local module inside our repo? Are we not able to move this up and combine it with our top level package.json? Also, what is the main point of this module being here? You've probably gone over this before, but I forgot.

We have this local module because our back-end is comprised of AWS lambdas and those require an isolated bundle of code to deploy. Because we don’t fully control our bundle packing and deployment stuff, we have to ensure that everything imported to a lambda is part of a module / package which can be pulled into the bundle. Importing anything using relative file paths in a lambda would introduce a linkage that would break when bundling happens because the reference file won’t be pulled into the js bundle.

I kinda include everything that's not code related to displaying a view as business logic, which is probably incorrect LOL. and also why we have different understandings of what business logic is. Tbh I mentioned classes because that's what my co-op does for their services. They use class-based services to make API calls and also to house related helper functions (like transformers). I did not completely research this section so I can look more into it, but having it be class-based looks nicer in my opinion than just having a file with a bunch of functions (albeit the class thing is just boiler plate code and it wouldn't take in any constructor params or anything).

That makes sense! I think of business logic as being things that encode the business processes. Things like “timeline status is calculated by ….” or “duration of a project is calculated as latest work package end date minus earliest work package start date”.

I think class-based could definitely be nicer, but I’m not sure, so I’d be curious to hear how you want to do it. Our functional system has grown in a fractured method, so perhaps switching to class-based and standardizing what we need could be useful.

@anthonybernardi anthonybernardi transferred this issue from Northeastern-Electric-Racing/PM-Dashboard-v2 Aug 11, 2022
@anthonybernardi
Copy link
Collaborator

we implemented most of this yahoo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spike Research
Projects
None yet
Development

No branches or pull requests

4 participants