Introduce custom ESLint rules to apply and enforce new themed component selector convention

[ybnd]

References

Add references/links to any related issues or PRs. These may include:

• Fixes #2845

Description

This PR introduces custom ESLint plugins as a means of automatically migrating to the new themed component selector convention proposed in #2845, as well as enforcing this convention moving forward.

Furthermore, these plugins can be leveraged in future PRs to create more rules for DSpace Angular in particular.

Note that this PR also enables linting for our Cypress tests, since they are affected by the selector change (most of the changes there are indentation/import order).

Instructions for Reviewers

• The first commit of this PR includes the bulk of the new rules, enough to cover the majority of cases
  • It feels right to bundle these plugins in the project itself, since they are tightly coupled to DSpace Angular.
    • Do you agree? Any comments on the structure?
  • The new plugins do not add more than a few seconds to the total linting time.
    • You can re-confirm this by running TIMING=1 yarn run eslint . --quiet
    • The dspace-angular-ts/themed-component-usages rule is the most impactful -- it may be useful to optimize it further, but it won't have as much as an effect as #2864
  • Please observe the lint output in the GitHub build logs, or locally if you check out this commit. Are the messages sufficiently clear?
• The "auto-migrate" commit includes only the results of yarn lint --fix. The manual interventions can be found in the subsequent commits
  • These commits can serve as an example of what downstream forks will have to do to migrate to the new convention (but likely a lot less than was necessary in this PR)
  • Is this reasonable? Do we need more detailed instructions?
• The app should remain functionally the same
  • Run locally to confirm
  • Check a few themeable components to confirm that
    • The new convention is correctly reflected in the DOM
    • Everything still renders exactly as before
  • Switch between the dspace and custom themes
• If you're using Windows, I'd greatly appreciate if you check whether all new yarn scripts work there as well (they have already been tested on MacOS and Linux).

Instructions for fork developers

After upgrading your fork to DSpace 8.0, run yarn lint --quiet --fix

• This should automatically migrate any custom themeable/themed components in your project
• Build/test/deploy your project and verify that everything still works
  • If there were major problems with the migration, they will come up as compilation errors. These can include
    • Out-of-sync inputs between the base themeable component and its ThemedComponent wrapper
    • ...
  • Please open an issue on GitHub if the cause of the problem is not clear

Future work

I've already worked on similar ESLint rules in a separate project; if we decide to merge this PR it may be useful to port them to the "internal" lint plugin:

• Enforce correct theme declarations in entry component decorators such as @listableObjectComponent
• Enforce globally unique entry component decorators (to avoid silent overrides)
• Enforce import aliases (e.g. of as observableOf)

And moving forward there are many other DSpace-isms that we could write rules for

• Ensure that the inputs and outputs of ThemedComponent wrappers match the base component exactly
• Ensure that component overrides in themes do not introduce new inputs or outputs
• ...

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• [x] My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• [x] My PR passes ESLint validation using yarn lint
• [x] My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
• [ ] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• [x] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• [x] If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• [x] If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• [x] If my PR fixes an issue ticket, I've linked them together.

[ybnd]

Still some e2e failures, but I'm pretty sure that those tests were already flaky

[github-actions[bot]]

Hi @ybnd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

[ybnd]

Note: by far most of the conflicts will be due to the automatic lint fixes, so I'll address them in one go when the PR is approved.

Probably best to drop https://github.com/DSpace/dspace-angular/pull/2865/commits/3753b29a7313c676d4c9c720a3a7b5be0bfa087d, https://github.com/DSpace/dspace-angular/pull/2865/commits/d15327cd1301c04ee9f22868506ead34baed2574, https://github.com/DSpace/dspace-angular/pull/2865/commits/db7a371d332155f641ef04f02b8d80398484c7bc and re-do everything at the end of the branch for clarity.

[github-actions[bot]]

Hi @ybnd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

[tdonohue]

Hi @ybnd : This has conflicts because we just merged the Angular 16 upgrade in #2871 . If you have time to resolve the conflicts, both @atarix83 and I were planning on trying this out next week during testathon. We likely won't be able to review this prior to testathon though, unfortunately.

[ybnd]

@tdonohue @atarix83 done.

There were only very minor conflicts; I'll keep an eye out in case there's more in the next few days

[github-actions[bot]]

Hi @ybnd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

[ybnd]

Fixed e2e test failure and tacked on a workaround for #2946

[ybnd]

@atarix83 I'm using IntelliJ IDEA, so ESLint support is probably the same in WebStorm. To use these plugins you'll need to run yarn build:lint first. I already included it in the yarn lint step, but it should really be documented more clearly...

[ybnd]

@atarix83 just had the same exact idea, so I'm all for it 👍

I think it makes sense to remove the build step from yarn lint itself too, so we don't re-build the plugins each time

[github-actions[bot]]

Hi @ybnd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

[tdonohue]

@ybnd : Per @atarix83 's note above, I looked into the Docker build issues in this PR. It appears this PR is currently throwing a build error when it attempts to build dspace-angular-dist. Here's the error:

#11 71.98 $ yarn build:lint
#11 72.14 yarn run v1.22.19
#11 72.17 $ rimraf 'lint/dist/**/*.js' 'lint/dist/**/*.js.map' && tsc -b lint/tsconfig.json
#11 72.41 error TS5083: Cannot read file '/app/lint/tsconfig.json'.
#11 72.42 error Command failed with exit code 1.
#11 72.42 info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

The Docker build uses [src]/Dockerfile.dist as shown here: https://github.com/DSpace/dspace-angular/blob/main/.github/workflows/docker.yml#L41-L52

It's unclear to me if that Dockerfile.dist requires updates, or if this indicates an unexpected bug/behavior in the build process with this PR.

[ybnd]

@tdonohue @atarix83 I'm not super familiar with the Docker setup, but it seems like it involves a yarn install with only package.json & yarn.json, so it's failing during the new postinstall script.

Added a condition to check whether the lint plugin sources are present before trying to build, let's see if that's enough 🤞

Edit: @artlowel pointed out that that wouldn't be cross-platform, so I changed it to "skip if failed" so we can avoid introducing a new Node dependency just for this one tiny thing.

[tdonohue]

@ybnd : Apologies, I've since discovered that some of the bugs that I've run into are not the cause of this PR. The custom theme doesn't work on main either, so I logged a bug ticket https://github.com/DSpace/dspace-angular/issues/2977

Unfortunately though, that makes it difficult to fully test this PR until that ticket is fixed. In the meantime though, all the yarn script failures on Windows obviously seem specific to this PR.

[ybnd]

@tdonohue I've addressed the issues with the new yarn scripts on Windows, so this should be ready to review again once https://github.com/DSpace/dspace-angular/issues/2977 is resolved

[github-actions[bot]]

Hi @ybnd, Conflicts have been detected against the base branch. Please resolve these conflicts as soon as you can. Thanks!

[tdonohue]

@ybnd : This has minor conflicts caused by the fix to the custom theme: #2984 Once those conflicts are resolved, I should be able to test this again (hopefully on/around Monday)

[ybnd]

@tdonohue couldn't make Monday, hopefully today is still ok timing-wise.