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

Example Of Refactoring To Make Files More Descriptive and use jsx instead of js #929

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DysektAI
Copy link
Contributor

1.) There are a LOT of "index.js" files being used to define components as I never seen this before in my time of using react unless using index.js to import multiple components into one file for easier use and access here is the best resource I found that explains this:

  1. Don’t use index.js as an entry point to a directory
    When you use index.js as an entry point to a directory, it can be difficult for other developers to understand the purpose of the file and how it fits into the overall project structure. This is because index.js files are often used as generic catch-all files that contain code from multiple components or modules.

Instead, try to name your files according to their purpose. For example, if you have a component called “MyComponent”, then name the file “MyComponent.js” instead of “index.js”. This will make it easier for other developers to quickly identify what each file does and where it belongs in the project structure.
Source: https://climbtheladder.com/10-react-naming-conventions-best-practices/

2.) Properly use .js and .jsx as explained below which currently is mostly .js extensions in the project.
(The reason why this isn't an apparent issue when using CRA is because it uses webpack and babel under the hood to handle these files no differently)

Vite's official documentation and community discussions provide insights on how Vite handles .jsx files differently from .js files, emphasizing the importance of using the correct file extensions for proper processing and optimization. This helps ensure that the JSX syntax is correctly transformed during the build process.
Reference: vitejs/vite#3448 (comment)

@DysektAI DysektAI requested a review from a team as a code owner April 23, 2024 10:05
@Shebuka
Copy link
Contributor

Shebuka commented Apr 23, 2024

  1. Background: historically it's (was?) a CRA convention. In the code when importing you reference components by their folder name, never by the js file, and then i_dont_know_who_specificaly will look for the index.js file inside and the exported parts in it. Referencing imports by the js file is a relatively new thing required by ECMAScript.
    Because we recently migrated to ECMAScript (784a6c1 it was pain 🤪) it can be useful to also migrate the naming convention 👍
    Discussion points:
    • Should the folder be renamed too or the file named as folder?
    • Do we need a folder if we only have the jsx file inside (without CSS)?
  2. Background: historically it's (was?) a CRA convention. Totaly should be done moving forward 👍

@DysektAI
Copy link
Contributor Author

DysektAI commented Apr 23, 2024

  1. Background: historically it's (was?) a CRA convention. In the code when importing you reference components by their folder name, never by the js file, and then i_dont_know_who_specificaly will look for the index.js file inside and the exported parts in it.

I wasn't familiar with this older CRA convention either. Understanding this helps clarify why it was done that way.


Referencing imports by the js file is a relatively new thing required by ECMAScript.

I've heard about the need for explicit extensions with ECMAScript, but haven't needed to implement it in my project, which still runs smoothly without specifying .js or .jsx extensions. (I did have issues when using package.json type: module removing it fixed a lot of the issues but to use Vite it was required and still don't see any issues maybe my ESLint isn't configured correctly)


Because we recently migrated to ECMAScript it can be useful to also migrate the naming convention 👍
Discussion points:

  • Should the folder be renamed too or the file named as folder?

I typically organize components like 'settings' under src/components/settings/ and then import them into src/pages/settings.jsx for rendering. This setup works well for my /settings route as I can add and remove components easily or simply edit those components in their respective files if needed.


  • Do we need a folder if we only have the jsx file inside (without CSS)?

For components using inline styling, especially with frameworks like MUI that use the sx prop, external CSS files aren't necessary. However, for those that do have associated CSS, structuring folders like this could be more organized:

/src
  /components
    /BarterTooltip
      BarterTooltip.jsx
  /styles
    /BarterTooltip
      BarterTooltip.css
  • Vite treats CSS like JavaScript modules, allowing faster style updates via ES module capabilities in browsers.
  • Webpack, while using loaders like style-loader and css-loader, bundles CSS with JavaScript, which can be less efficient during development. (Used by CRA)

  1. Background: historically it's (was?) a CRA convention. Totaly should be done moving forward 👍

Once the above are addressed at least for the most part, it would be beneficial to move forward with implementing Vite.

Switching to Vite would streamline workflow, significantly reduce server startup times and improve build speeds with efficient Hot Module Reloading (HMR).

Overall, transitioning to a clearer structure and simplifying the codebase will enhance the development process greatly.

@Shebuka
Copy link
Contributor

Shebuka commented May 14, 2024

@DysektAI I'll say that if you want to take on the monumental effort to rename all react components from js to jsx go for it 👍

This will prepare us better for when we are ready to switch to Vite or Next.js

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

Successfully merging this pull request may close these issues.

None yet

2 participants