Site-kit-wp: Use component name for component file names

Created on 9 Jun 2020  ·  13Comments  ·  Source: google/site-kit-wp

Feature Description

As decided recently, JavaScript components should have their respective file or folder names match the component name. For example, a component DashboardPageSpeed should be in either DashboardPageSpeed.js or DashboardPageSpeed/index.js.

Another change specific to Site Kit module components is that all components should be within assets/js/modules/{module}/components instead of assets/js/modules/{module}.

As part of this issue, (almost) all refactored components should be renamed/moved to match these standards. To not go completely overboard with this work and to ensure it doesn't touch too many files we may soon remove anyway, this issue should specifically focus on anything in assets/js/googlesitekit and assets/js/modules.


_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

  • All module components should be found within assets/js/modules/{module}/components. Module directories that only have components should become subdirectories of that general components directory.
  • All components within assets/js/googlesitekit and assets/js/modules should have their file names (or directory names, where applicable) changed to match the component name (plus .js of course 😄 ).

Implementation Brief

  • npm install --save-dev eslint-plugin-filenames in the repo
  • Add "filenames" to eslintrc.json's plugins section
  • Add checks for all files in a components/ folder with ESLint's overrides:
    {
      "files": [
        "**/components/**/*.js"
      ],
      "rules": {
        "filenames/match-exported": [
          2,
          "pascal"
        ]
      }
    },
  • Fix all _component_ names flagged by ESLint. Tested this and it looks like there shouldn't be any files flagged that aren't legitimate errors, so no overrides should be needed. If any overrides _are_ needed though (eg I missed something checking the ouput of npm run lint:js: feel free to add them 😄

QA Brief

Changelog entry

  • N/A
Good First Issue P1 Eng Enhancement

Most helpful comment

@felixarntz Have you considered enforcing this with ESLint? See https://github.com/selaux/eslint-plugin-filenames#matching-exported-values-match-exported for a possible solution

All 13 comments

@felixarntz Have you considered enforcing this with ESLint? See https://github.com/selaux/eslint-plugin-filenames#matching-exported-values-match-exported for a possible solution

@tofumatt IB sounds good overall, however I wonder whether we should do it the other way around and only apply this file name linting rule to files that are within a components folder? There are some components where this is currently not the case, and some non-components that are in a components folder, but we could add exceptions for those or then move them somewhere more appropriate.

I've amended the overrides in the IB to use:

        "**/datastore/**/*.js",
        "**/util/**/*.js",
        "**/utils/**/*.js",
        "tests/**/*.js"
```.

Testing with that it produces very few—if any—errors that we would want to ignore, but it catches a LOT of components not currently in `components/` folders, like:

assets/js/modules/tagmanager/setup/setup.legacy.js
assets/js/modules/search-console/settings/settings-view.js
assets/js/modules/analytics/settings/settings-view.js
```

etc. So I think it's best to start with casting a wide net and go from there 😄

@tofumatt I think we should rather build this linting for how our files _should_ be organized rather than for how they are organized now. Every component file should be somewhere within a components folder and we want only those files to have this kind of name, so I think it would be better to only apply the rule to those files, i.e. opt in for components rather than opt out for non-components.

For those not covered but should be covered, we could then add these folders specifically to be opted in as well.

@tofumatt Regarding linting the IB is good, however it only addresses the second point of the ACs. I guess an IB for how to move files to a components folder isn't _really_ needed, but let's make sure that is covered in the PR.

(IB ✅ )

@jqlee85 #1773 is merged - ready for the next one 👍

@jqlee85 #1791 merged ✅

@jqlee85 Back to you - the missing piece now is assets/js/components right?

@felixarntz, I believe this issue was scoped to just modules and assets/js/googlesitekit. (From above:)

To not go completely overboard with this work and to ensure it doesn't touch too many files we may soon remove anyway, this issue should specifically focus on anything in assets/js/googlesitekit and assets/js/modules.

But I can certainly expand to do the assets/js/components as well. Let me know if I should go ahead and continue with that.

@jqlee85 Oh yeah, you're right my bad. This is good for QA then 👍

Overall everything looks good but I have noticed that there are directories with components that either lack index.js file or have it with insufficient list of exported components. I think that we need to make it consistent accross all modules and directories. @felixarntz / @aaemnnosttv do you think we need to handle it in the scope of this ticket or we need to create a new one? If you think it should be done in a separate ticket, then we can consider this ticket as passed QA and as can be moved to the Approval column.

Directories with insufficient index files:

  • adsense/components/dashboard
  • analytics/components/settings
  • optimize/components/settings
  • pagespeed-insights/components/settings
  • search-console/components/settings
  • tagmanager/components/common

Directories without index files:

  • analytics/components/adminbar
  • analytics/components/dashboard-details
  • analytics/components/wp-dashboard
  • pagespeed-insights/components/common
  • pagespeed-insights/components/dashboard
  • search-console/components/adminbar
  • search-console/components/dashboard-details
  • search-console/components/dashboard
  • search-console/components/wp-dashboard

@eugene-manuilov Good catch! Can you open a separate issue for addressing this?

@felixarntz done: #1838. Moving it to the Approval column.

Was this page helpful?
0 / 5 - 0 ratings