[ESLint plugin] Add a rule to enforce folder architecture on frontend #7481

gitstart-app · 2024-10-07T19:47:13Z

Description

### **How String Validation Works**

The validateString function is responsible for validating filenames based on configuration:

1. Configuration definition

- You defined one configuration by using 3 keys: name, children, and ruleId

2. RuleId

- Rule ID is a key for a reusable configuration that allows us to avoid repetition and each rule can also refer to other rules and itself, that makes the recursive behavior work.

3. Children

- With children prop you define the name patterns of the subfolders or files inside the current folder, a configuration with children is considered a folder, The reusable rules are also folder definition

4. **Prefix, Suffix, and Name Pattern**:

- The name property is the core of the validation, It’s an object with the definition of the required `prefix`, `suffix`, or `extension`, and the `namePattern` itself

    - note that each one of these properties can be a string or an array of strings

    - and for `namePattern`, we support special values that are computed to regex validators (e.g., `PascalCase`, `camelCase`)

    - if the or folder just needs to shame a simple name, you can pass a string instead of an object to `name`

- The function checks if a file or folder name starts with one of the prefixes, ends with one of the suffixes, has one of the extensions, and matches the `namePattern`

5. **Wildcard Handling**:

- When `namePattern` is `"*"`, the function returns `true` immediately, allowing any name without further validation.

6. The checkFolderStructure function checks the folder name, and if there is a ruleId calls itself passing the object that represents the rule id, if there is the children prop we split the rules for folders and files and check all of the folders and files against every corresponding rule until one of them passes

7. The folder is considered valid if It’s name and all of the sub-files and sub-folders names are valid.

8. We store the validations in an object and then we pass the result to the eslint reporter

---

### **Why hasRun Check is Needed**

The hasRun flag is used to prevent the validation process from running multiple times. Running this rule multiple times, especially on large folder structures, would have a significant performance impact due to repeated validation.

```tsx

let hasRun = false;

if (hasRun) return {};

hasRun = true;

```

The hasRun check ensures that we avoid redundant validation and maintain efficiency.

The library that we were using was running for each eslint event triggered (based on each file), so that’s why the commands were not finishing because the eslint was triggering the check multiple times and we have too many files and folders to check each time.

this causes the error to be computed for the first eslint file that triggers the event even if this file passes in the test, but in the reporter, we are passing the path that is invalid

Fixes: #7329

greptile-apps

PR Summary

This PR introduces an ESLint plugin to enforce a consistent folder structure in the Twenty frontend project.

Added eslint-plugin-project-structure dependency in package.json
Implemented folder structure rules in new file packages/twenty-front/project-structure.cjs
Configured ESLint to use the new folder structure rules in packages/twenty-front/.eslintrc.cjs
Set rule to 'warn' mode, allowing for gradual adoption without breaking existing builds

_{3 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

gitstart-twenty

Hello @ijreilly 👋 , we were almost finishing this PR but a problem arose, after adding the configurations to the modules folder, eslint is taking a long time to perform the check, as this involves checking many folders, we are investigating this performance issue. That's why we sent the PR as a draft, to see how it would behave in the CI. But we also have some highlighted points about the structure that would be nice if you could clarify, I marked these points in the comments below.

packages/twenty-front/project-structure.cjs

ijreilly · 2024-10-08T08:31:10Z

I will let @charlesBochet answer to you !

charlesBochet · 2024-10-08T09:18:07Z

packages/twenty-front/project-structure.cjs

+      children: [
+        {
+          name: 'twenty-front',
+          children: [


i'm not sure we should talk about the files out of the src/modules folder for now, is there a way not taking them into account at all?

packages/twenty-front/project-structure.cjs

charlesBochet

@gitstart-twenty Thanks for the PR, let's only focus on the modules pages, make the rule a warning and treat / discuss edge cases in other PRs

charlesBochet · 2024-10-11T08:22:29Z

@gitstart-twenty I'm going to close this one to keep the history clean, feel free to re-open it whenever you are ready

charlesBochet · 2024-10-15T12:24:17Z

@gitstart-twenty is this ready for review?

tools/eslint-rules/rules/folderStructureConfig.ts

gitstart-twenty

Hello @ijreilly Hello @charlesBochet
since we managed to improve the performance we are adding the entire frontend folder in the configuration, I've left a explanation on the description about it.

I'm sending the comments to discuss again from here

gitstart-twenty · 2024-10-15T12:46:57Z

tools/eslint-rules/rules/folderStructureConfig.ts

+      // TODO: edge cases for subfolders of __stories__ that are also stories folders
+      /* 
+        - /src/pages/settings/developers/__stories__/api-keys
+        - /src/pages/settings/developers/__stories__/webhooks 
+        - /src/pages/settings/data-model/__stories__/SettingsObjectNewField
+
+        */
+      // should we enforce StrictPascalCase?
+      { name: 'kebab-case', ruleId: 'StorybookFolder' },
+      { name: 'StrictPascalCase', ruleId: 'StorybookFolder' },
+      { name: 'perf', ruleId: 'StorybookFolder' },
+      { ruleId: 'StorybookFolder' },
+    ],
+  },


gitstart-twenty · 2024-10-15T12:47:13Z

tools/eslint-rules/rules/folderStructureConfig.ts

+      {
+        name: {
+          namePattern: ['StrictPascalCase', 'camelCase', 'kebab-case'],
+          // TODO:  SVG extensions is a edge case for list-view-grip.svg


gitstart-twenty · 2024-10-15T12:48:15Z

tools/eslint-rules/rules/folderStructureConfig.ts

+  UtilsFolder: {
+    name: 'utils',
+    children: [
+      // TODO: what is the correct rule for utils?


we are accepting everything now, but we can define a pattern

gitstart-twenty · 2024-10-15T12:49:19Z

tools/eslint-rules/rules/folderStructureConfig.ts

+          {
+            name: {
+              namePattern: ['StrictPascalCase', 'camelCase', 'kebab-case'],
+              // TODO: test.tsx extensions is a edge case, because the files that end with test.tsx, infact do not use tsx


gitstart-twenty · 2024-10-15T12:49:35Z

tools/eslint-rules/rules/folderStructureConfig.ts

+      {
+        name: {
+          namePattern: ['StrictPascalCase', 'camelCase', 'kebab-case'],
+          // TODO: getFiledButtonIcon.tsx, assertWorkflowWithCurrentVersionIsDefined.tsx extensions is a edge case, because the files that end .tsx, infact do not use tsx


gitstart-twenty · 2024-10-15T12:50:05Z

tools/eslint-rules/rules/folderStructureConfig.ts

+                // src/loading/__stories__
+                {
+                  name: '__stories__',
+                  // TODO: should we also include the Loader Suffix here? we have PrefetchLoading.stories in the folder


gitstart-twenty · 2024-10-15T12:50:33Z

tools/eslint-rules/rules/folderStructureConfig.ts

+                { ruleId: 'StorybookFolder' },
+                // TODO: this is an edge case for ComponentFolderWithStories
+                // This is the only folder that breaks the recursive rule for page/settings
+                {
+                  name: 'data-model',
+                  children: [
+                    { ruleId: 'UtilsFolder' },
+                    { ruleId: 'TypesFolder' },
+                    { ruleId: 'HooksFolder' },
+                    { ruleId: 'ConstantsFolder' },
+                    { ruleId: 'StorybookFolder' },
+                    {
+                      name: 'SettingsObjectNewField',
+                      ruleId: 'ComponentsGroup',
+                    },
+                    { ruleId: 'ComponentsGroup' },


gitstart-twenty · 2024-10-15T12:50:42Z

tools/eslint-rules/rules/folderStructureConfig.ts

+                    { ruleId: 'ComponentsGroup' },
+                  ],
+                },
+                // TODO: Edge case because SettingsCRMMigration is a folder and does not pass on StrictPascalCase


gitstart-twenty · 2024-10-15T12:51:43Z

tools/eslint-rules/rules/folderStructureConfig.ts

+        // TODO: is it fine the way we are handling this?
+        {
+          name: {
+            namePattern: 'kebab-case',
+            extension: 'd.ts',
+          },
+        },
+        {
+          name: {
+            namePattern: 'index',
+            extension: 'css',
+          },
+        },
+        {
+          name: {
+            namePattern: 'index',
+            extension: 'tsx',
+          },
+        },
+        {
+          name: {
+            namePattern: 'App',
+            extension: 'tsx',
+          },
+        },
+        {
+          name: {
+            namePattern: 'SettingsRoutes',
+            extension: 'tsx',
+          },


we could accept any file, but since there are few files, we are listing here

gitstart-twenty · 2024-10-15T12:51:53Z

tools/eslint-rules/rules/folderStructureConfig.ts

+                namePattern: [
+                  'kebab-case',
+                  'StrictPascalCase',
+                  // TODO: Edge case for windows11 folder


greptile-apps

PR Summary

(updates since last review)

This PR introduces a new ESLint rule to enforce folder architecture on the frontend, along with significant changes to the codebase structure and component implementations.

Implemented custom ESLint rule in folder-structure-rule.ts to validate folder and file names against predefined patterns
Removed several field-specific components (e.g., EmailFieldDisplay, LinkFieldInput) in favor of more generic implementations
Refactored action menu and context menu functionality throughout the application, replacing context menus with action menus in many components
Updated object metadata handling, including new types and utilities for composite and non-composite field types
Improved responsiveness and layout of various components, particularly in settings and record table areas

_{299 file(s) reviewed, 274 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

charlesBochet

@gitstart-twenty I think the overall approach is good but this is way too detailed and likely unmaintainable :(
Can we focus on the modules and pages folder only.

Modules:
Good example: packages/twenty-front/src/modules/object-record/record-table

Pages:
Good example: packages/twenty-front/src/pages/auth
should only have .tsx filed in it and no subfolders (except stories)

Regarding the case, it is not tied to the folder architecture and it is actually way simpler:
.tsx ==> PascalCase
.ts => camelCase

I believe this should be checked by another rule TBH

The suffix thing is interesting to check and is folder related but let's not be overfit there too: checking that stories only contains .stories.tsx makes sense, checking for .svg does not at this point

gitstart-twenty · 2024-10-17T18:24:09Z

Thank you for the update @charlesBochet
We added the .svg there so the command wouldn't fail and you could choose what we should do with that file.

Ignoring the other folders, we have some pending confirmations about the changes we should make in the next PRs:

timelineActivities breaks the modules name pattern
What would be the default shape for utils folders?
Within some utils folders there are subfolders of tests, and some files that are tested use the tsx extension when they do not actually use tsx
Subfolders of "stories" folder that are also stories folders, like:
- /src/pages/settings/developers/stories/api-keys
- /src/pages/settings/developers/stories/webhooks
- /src/pages/settings/data-model/stories/SettingsObjectNewField
- and perf folders as well
Should we enforce a specific pattern for these cases? in other locations, this is usually PascalCase
What should we do with /src/modules/ui/input/components/list-view-grip.svg, move or accept that it should be there?
isMobile.test.tsx breaks the hooks folder rule because it should start with use
pages/data-model is not a folder with components and storybook subfolders, should we continue like this?
We have currently added a separate rule to handle to this folder
SettingsCRMMigration is not a strict PascalCase should we accept it as is or should no file be a strict Pascal case?

charlesBochet · 2024-10-17T18:37:49Z

Thanks! Generally as the rule should be a warning it's OK if the current state of modules and pages folder are not perfect yet. Let's not make the rule fit the folders, we will make the folders fit the rule piece by piece

We added the .svg there so the command wouldn't fail and you could choose what we should do with that file. ==> it's OK if it issues a warning

Ignoring the other folders, we have some pending confirmations about the changes we should make in the next PRs:

timelineActivities breaks the modules name pattern ==> should be timeline-activities but let's leave it like this, will issue a warning
What would be the default shape for utils folders? should only contain __tests__ + camelCase.ts (but the case is tied to the .ts extension not to the fact it's in the utils folder
Within some utils folders there are subfolders of tests, and some files that are tested use the tsx extension when they do not actually use tsx ==> that's fine, that's another issue we can fix that later :) (create an issue maybe)
Subfolders of "stories" folder that are also stories folders, like:
- /src/pages/settings/developers/stories/api-keys
- /src/pages/settings/developers/stories/webhooks
- /src/pages/settings/data-model/stories/SettingsObjectNewField
- and perf folders as well
  ==> that's wrong, stories should be at the very bottom. Fine to issue a warning for now
What should we do with /src/modules/ui/input/components/list-view-grip.svg, move or accept that it should be there? ==> should be a warning for now, this should be in an assets folder I guess
isMobile.test.tsx breaks the hooks folder rule because it should start with use ==> true, should issue a warning
pages/data-model is not a folder with components and storybook subfolders, should we continue like this?
We have currently added a separate rule to handle to this folder ==> pages should not follow the modules rule, they should only have .tsx and no subfolders except for __stories__
SettingsCRMMigration is not a strict PascalCase should we accept it as is or should no file be a strict Pascal case? ==> should issue a warning

gitstart-twenty · 2024-10-17T18:55:00Z

ok, thanks for the clarifications @charlesBochet 🤝
one question, when you say:

" stories_" should be at the very bottom

you mean that " stories_" should not have sub folders, rigth?

charlesBochet · 2024-10-17T19:12:52Z

ok, thanks for the clarifications @charlesBochet 🤝 one question, when you say:

" stories_" should be at the very bottom

you mean that " stories_" should not have sub folders, rigth?

Yes :)

tools/eslint-rules/rules/folderStructureConfig.ts

@@ -0,0 +1,396 @@
+export const CASES: Record<string, RegExp | undefined> = {
+  'kebab-case': /^[a-z]+(-[a-z]+)*$/,
+  StrictCamelCase: /^[a-z]+((\d)|([A-Z0-9][a-z0-9]+))*([A-Z])?$/,


To fix the problem, we need to remove the ambiguity in the regular expression that causes exponential backtracking. The best way to do this is to refactor the regular expression to avoid nested quantifiers and ensure that each part of the pattern can only match in one way.

For the regular expression ^[a-z]+((\d)|([A-Z0-9][a-z0-9]+))*([A-Z])?$, the problematic part is ((\d)|([A-Z0-9][a-z0-9]+))*. We can refactor this to avoid ambiguity by ensuring that each part of the pattern is distinct and does not overlap in what it can match.

tools/eslint-rules/rules/folderStructureConfig.ts

@@ -0,0 +1,396 @@
+export const CASES: Record<string, RegExp | undefined> = {
+  'kebab-case': /^[a-z]+(-[a-z]+)*$/,
+  StrictCamelCase: /^[a-z]+((\d)|([A-Z0-9][a-z0-9]+))*([A-Z])?$/,


To fix the problem, we need to modify the regular expression to remove the ambiguity that causes exponential backtracking. Specifically, we can change the sub-expression [a-z0-9]+ to a more precise pattern that avoids ambiguity. One way to achieve this is by using a non-capturing group with a more specific pattern.

General Fix: Modify the regular expression to eliminate ambiguous patterns that can cause backtracking.

Detailed Fix: Change the regular expression on line 3 to use a non-capturing group and a more specific pattern to avoid ambiguity.

Specific Changes: Update the regular expression in the CASES object on line 3.

Requirements: No additional methods, imports, or definitions are needed.

tools/eslint-rules/rules/folderStructureConfig.ts

+export const CASES: Record<string, RegExp | undefined> = {
+  'kebab-case': /^[a-z]+(-[a-z]+)*$/,
+  StrictCamelCase: /^[a-z]+((\d)|([A-Z0-9][a-z0-9]+))*([A-Z])?$/,
+  StrictPascalCase: /^[A-Z](([a-z0-9]+[A-Z]?)*)$/,


To fix the problem, we need to modify the regular expression to remove the ambiguity that causes exponential backtracking. Specifically, we can change the nested quantifiers to ensure that the pattern is more deterministic. One way to achieve this is by using a non-capturing group and making the inner repetition non-greedy.

Modify the regular expression on line 4 to avoid nested quantifiers that can cause exponential backtracking.

Change [a-z0-9]+ to [a-z0-9]* and adjust the overall pattern to ensure it still matches the intended strings without causing performance issues.

gitstart-twenty · 2024-10-18T17:35:28Z

Hello @charles, we updated the PR.

One thing that we did not update for the hooks folder is the test.tsx extension because tests for hooks usually need TSX because of providers (Recoil and Apollo),
but the warning triggered for files that use .tsx extensions outside the tests will be applied

we also included better regexes to check the file names with strict Pascal and strict Camel case more precisely (we check the AI suggestion after pushing, they make the validation not strict), along with the check of the patterns you suggested here:

.tsx ==> PascalCase
.ts => camelCase (we also included kebab case here)

this causes a lot of warnings to be shown, we are not sure if you want to keep it or if you want to define some exception

in total we have almost 400 warnings

charlesBochet · 2024-10-19T17:46:49Z

@gitstart-twenty I have taken a deep look at your PR before merging it and wanted to double check that we could not use eslint-plugin-project-structure out of the box. Using this library avoids us to maintain our own rule (which I think is over-complex)

I've managed to implement it in a simpler way than your proposal in: #7863

Is there anything I'm missing?

charlesBochet · 2024-10-19T17:49:52Z

i'll close this PR, I'm sorry but this does not meet our quality standard

gitstart-twenty · 2024-10-21T11:40:33Z

Hey @charlesBochet
we initially used the library but there was a performance issue, that made the eslint command never end, I see that now they pushed other version's updates, they should fixed in one of these versions

charlesBochet · 2024-10-21T12:26:51Z

I think there was an infinite loop in their camelCase regex, that's why i have overriden it (see on main)

greptile-apps bot reviewed Oct 7, 2024

View reviewed changes

gitstart-twenty marked this pull request as draft October 7, 2024 19:55

gitstart-twenty reviewed Oct 7, 2024

View reviewed changes

charlesBochet reviewed Oct 8, 2024

View reviewed changes

packages/twenty-front/project-structure.cjs Outdated Show resolved Hide resolved

charlesBochet reviewed Oct 8, 2024

View reviewed changes

charlesBochet added the -PR: awaiting author label Oct 8, 2024

charlesBochet self-assigned this Oct 8, 2024

charlesBochet added the -PR: wip label Oct 8, 2024

charlesBochet closed this Oct 11, 2024

gitstart-twenty reopened this Oct 15, 2024

[ESLint plugin] Add a rule to enforce folder architecture on frontend

790e897

gitstart-app bot force-pushed the TWNTY-7329 branch from bf55d6e to 790e897 Compare October 15, 2024 12:35

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

tools/eslint-rules/rules/folderStructureConfig.ts Fixed Show fixed Hide fixed

gitstart-twenty reviewed Oct 15, 2024

View reviewed changes

gitstart-twenty added -PR: awaiting review and removed -PR: wip -PR: awaiting author labels Oct 15, 2024

gitstart-twenty requested review from charlesBochet and ijreilly October 15, 2024 12:53

gitstart-twenty marked this pull request as ready for review October 15, 2024 12:53

greptile-apps bot reviewed Oct 15, 2024

View reviewed changes

charlesBochet reviewed Oct 17, 2024

View reviewed changes

charlesBochet added -PR: awaiting author and removed -PR: awaiting review labels Oct 17, 2024

gitstart-twenty added 2 commits October 18, 2024 17:09

Merge branch 'main' into TWNTY-7329

0d8a88f

update rules and show ESLint warnings

420217f

github-advanced-security bot found potential problems Oct 18, 2024

View reviewed changes

gitstart-twenty added -PR: awaiting review and removed -PR: awaiting author labels Oct 18, 2024

Merge branch 'main' into TWNTY-7329

25ad547

charlesBochet closed this Oct 19, 2024

[ESLint plugin] Add a rule to enforce folder architecture on frontend #7481

[ESLint plugin] Add a rule to enforce folder architecture on frontend #7481

Conversation

gitstart-app bot commented Oct 7, 2024 • edited Loading

Description

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

gitstart-twenty left a comment

Choose a reason for hiding this comment

ijreilly commented Oct 8, 2024

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

charlesBochet commented Oct 11, 2024

charlesBochet commented Oct 15, 2024

gitstart-twenty left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

charlesBochet left a comment

Choose a reason for hiding this comment

gitstart-twenty commented Oct 17, 2024 • edited Loading

charlesBochet commented Oct 17, 2024 • edited Loading

gitstart-twenty commented Oct 17, 2024

charlesBochet commented Oct 17, 2024

gitstart-twenty commented Oct 18, 2024

charlesBochet commented Oct 19, 2024

charlesBochet commented Oct 19, 2024

gitstart-twenty commented Oct 21, 2024

charlesBochet commented Oct 21, 2024

gitstart-app bot commented Oct 7, 2024 •

edited

Loading

gitstart-twenty left a comment •

edited

Loading

gitstart-twenty commented Oct 17, 2024 •

edited

Loading

charlesBochet commented Oct 17, 2024 •

edited

Loading