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 Replace original selector when making a component themeable #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 Optimize linting performance by disabling "heavy" warning rules #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!

• 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.
• My PR passes ESLint validation using yarn lint
• 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.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• 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.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• 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]

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 3753b29, d15327c, db7a371 and re-do everything at the end of the branch for clarity.

[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.

[artlowel]

Thanks @ybnd that looks good!

@tdonohue it would be good if you could still run a sanity check to verify the build works as it should on windows, given the changes in 728b561

[tdonohue]

@ybnd : I've tested this today in production mode (yarn build:prod; yarn serve:ssr) on Windows 11. While it works for the dspace theme, the custom theme is broken. Here's what I see

There are a number of issues that are visible. The header no longer works, no ability to login or browse the site, much of the homepage content no longer displays, etc. Individual item pages are no better.

This doesn't appear to be a caching issue on my end, as after checkout of this PR I ran the following:

yarn clean
yarn install
yarn build:prod
yarn serve:ssr

I also attempted a hard reload in the browser in case this is a CSS issue.

I'll note that with the custom theme enabled, there are SSR errors that occur such as this one:

ERROR TypeError: Cannot read properties of undefined (reading 'onDestroy')
    at Object.ɵɵpipe (C:\dspace-angular\dist\server\main.js:1:4326830)
    at template (C:\dspace-angular\dist\server\1210.js:1:3854)
    at executeTemplate (C:\dspace-angular\dist\server\main.js:1:4182823)
    at renderView (C:\dspace-angular\dist\server\main.js:1:4198373)
    at renderComponent (C:\dspace-angular\dist\server\main.js:1:4198106)
    at renderChildComponents (C:\dspace-angular\dist\server\main.js:1:4198794)
    at renderView (C:\dspace-angular\dist\server\main.js:1:4198836)
    at ComponentFactory.create (C:\dspace-angular\dist\server\main.js:1:4235830)
    at R3ViewContainerRef.createComponent (C:\dspace-angular\dist\server\main.js:1:4239623)
    at Object.next (C:\dspace-angular\dist\server\main.js:1:2265629)

Finally, not all of the new yarn commands are working for me on Windows. Here's what works and what doesn't:

• yarn build:lint - Succeeds
• yarn test:lint - FAILS. I get two errors in the dspace-angular\lint\test\fixture\src\test.ts file.

  1) TypeScript rules themed-component-usages invalid disallow direct usages of base class
  Message:
  Error: Can't infer project-absolute TS/resource path from: C:\dspace-angular\lint\test\fixture\src\test.ts
  Occurred while linting C:\dspace-angular\lint\test\fixture\src\test.ts:2
  Rule: "themed-component-usages"
  
  2) TypeScript rules themed-component-usages invalid disallow direct usages of base class, keep other imports
  Message:
  Error: Can't infer project-absolute TS/resource path from: C:\dspace-angular\lint\test\fixture\src\test.ts
  Occurred while linting C:\dspace-angular\lint\test\fixture\src\test.ts:2
  Rule: "themed-component-usages"
  

• yarn test:lint:nobuild - FAILS with the same two errors (obviously)
• yarn lint - FAILS. I get 97 lint errors in the custom theme. All of them say something like "Unthemed version of themable component should have a selector starting with 'ds-base-'". Maybe this is why the "custom" theme is not working for me?
• yarn lint:nobuild - FAILS (same as yarn lint obviously)
• yarn lint-fix - Succeeds. It fixes all 97 errors above, but changes 97 files in the "custom" theme in the same way. Here's an example of the diff change (changed ds-themed-admin-sidebar to ds-base-themed-admin-sidebar):

  --- a/src/themes/custom/app/admin/admin-sidebar/admin-sidebar.component.ts
  +++ b/src/themes/custom/app/admin/admin-sidebar/admin-sidebar.component.ts
  @@ -15,7 +15,7 @@ import { AdminSidebarComponent as BaseComponent } from 
  '../../../../../app/admin
    * Component representing the admin sidebar
    */
   @Component({
  -  selector: 'ds-themed-admin-sidebar',
  +  selector: 'ds-base-themed-admin-sidebar'
  

• yarn docs:lint - Succeeds, but changes all line endings in the /docs/lint/** subdirectories to CRLF instead of LF. (This is not a serious issue...it's something we could live with if we had to)

While the overall code looks reasonable to me, this doesn't seem to be fully functional (at least not on Windows).

[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 #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 #2977 is resolved

[github-actions]

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.

[tdonohue]

👍 Thanks @ybnd ! I've retested this today and all the yarn scripts now work on Windows. I've also verified that theming is working as normal & I can still do things like configure a different theme per community/collection and the custom and dspace themes both work.

I'll merge this, but I'm also flagging it as needs documentation. I think this needs to have basic docs created in the Upgrading DSpace documentation as it seems like sites should now run yarn lint --fix whenever they upgrade? I may need your help with updating these docs so that they are accurate/complete. But if you'd rather provide the details on this PR, then I can copy them into the docs.