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

Configure ESLint rules for shared folders, bump ESLint deps #453

Merged
merged 1 commit into from Apr 1, 2022

Conversation

kachkaev
Copy link
Contributor

@kachkaev kachkaev commented Mar 31, 2022

🌟 What is the purpose of this PR?

@teenoh and I are moving UI components into [frontend]/src/shared/ui. We will be trying the new fractal tree approach for the folder structure to make codebase development more consistent and robust.

This PR configures two new ESLint rules for the future shared folders in frontend. It also upgrades ESLint-related dependencies given the opportunity.

⚠️ Known issues

  • The structure we are going to use is still experimental. It has been discussed with @CiaranMn, @benwerner01 and @teenoh; we have also used it in a recent internal project. Once our experiment is complete, we can re-asses the approach once again and apply it in more parts of the codebase, if agreed.

🐾 Next steps

  • Move primitive UI components to [frontend]/src/shared/ui, following the new file structure. This will enable UI development for the new Entity Editor: Entity editor header WIPΒ #445

  • Use kebab-case in [frontend]/src/pages to be able to lint local shared inside pages.

  • Consider converting the new ui folder into a package (we'll need that for blocks).

πŸ” What does this change?

  • New ESLint rule: "canonical/filename-no-index" – makes sure we don’t use index.ts(x) files in the new shared folder as this is against the new experimental file structure rules.

  • New ESLint rule: "unicorn/filename-case" – makes sure we use kebab-case only. camelCase is more error-prone.

  • All ESLint dependencies are updated.

πŸ“œ Does this require a change to the docs?

  • No

πŸ”— Related links

πŸ›‘ What tests cover this?

  • CI

❓ How to test this?

Check CI

πŸ“Ή Demo

With the new rules, creating index.ts(x) or helloWorld.ts will produce the following:

/path/to/hash/packages/hash/frontend/src/pages/[accountId]/entities/shared/helloWorld.ts
  1:1  error  Filename is not in kebab case. Rename it to `hello-world.ts`  unicorn/filename-case

/path/to/hash/packages/hash/frontend/src/util/shared/button/index.tsx
  1:1  error  'index.js' files are not allowed  canonical/filename-no-index

/path/to/hash/packages/hash/frontend/src/util/shared/hEllo.ts
  1:1  error  Filename is not in kebab case. Rename it to `h-ello.ts`  unicorn/filename-case

/path/to/hash/packages/hash/frontend/src/util/shared/index.ts
  1:1  error  'index.js' files are not allowed  canonical/filename-no-index

βœ– 4 problems (4 errors, 0 warnings)

@github-actions github-actions bot added area/blocks Relates to first-party blocks (area) area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/deps Relates to third-party dependencies (area) frontend labels Mar 31, 2022
Comment on lines +224 to +227
"@typescript-eslint/no-misused-promises": [
"error", // https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-misused-promises.md#checksvoidreturn
{ "checksVoidReturn": { "attributes": false, "properties": false } }
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to make this rule a bit more loose after upgrading "@typescript-eslint/eslint-plugin". See typescript-eslint/typescript-eslint#4650 and https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-misused-promises.md#checksvoidreturn for context. The rule is not less strict that it was.

Comment on lines +51 to 57
{
"files": ["**/shared/**/*"],
"rules": {
"canonical/filename-no-index": "error",
"unicorn/filename-case": "error"
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main part of the PR

@kachkaev kachkaev marked this pull request as ready for review March 31, 2022 22:06
@kachkaev kachkaev merged commit 9a45518 into main Apr 1, 2022
@kachkaev kachkaev deleted the ak/configure-linting-for-shared-folders branch April 1, 2022 08:13
@teenoh teenoh mentioned this pull request Apr 1, 2022
@nonparibus nonparibus added type/eng > frontend Owned by the @frontend team area/tests > integration New or updated integration tests and removed area: frontend labels Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/blocks Relates to first-party blocks (area) area/deps Relates to third-party dependencies (area) area/tests > integration New or updated integration tests type/eng > frontend Owned by the @frontend team
Development

Successfully merging this pull request may close these issues.

None yet

3 participants