dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

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

Open ybnd opened this issue 5 months ago • 6 comments

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 avatar Mar 14 '24 10:03 ybnd