CustomChecks type issues

[smncd]

Hi again!

I'm working on extending the types to include further definitions and compatibility with custom checks. https://github.com/smncd/sa11y/blob/extend-type-declarations/types/sa11y.d.ts.

I've run into a "bug" or issue with the way CustomChecks would currently be implemented in Typescript.

Currently this is how it would work:

// src/your-script.ts

import { Sa11y } from "sa11y";
import CustomChecks from "./custom-checks";

const sa11y = new Sa11y({
  customChecks: new CustomChecks // <--- Error
});

// -------------------------------------------------------------

// src/custom-checks.ts
import { Sa11y, Sa11yCustomChecks } from "sa11y";

export default class CustomChecks {
  sa11y: Sa11y; // <--- Error if constructor removed

  constructor(sa11y: Sa11y) {
    this.sa11y = sa11y;
  }

  setSa11y(sa11y) {
    this.sa11y = sa11y;
  }

  check() {}
}

This introduces a few errors.

• The CustomChecks constructor requires sa11y to be passed in during instantation, which isn't possible.
• If the above is fixed by removing the constructor, the sa11y property in CustomChecks complains that it's not defined in the constructor. This can be fixed by setting "strictPropertyInitialization": false in the project tsconfig.json, however this would have to be done on the "users" end, which isn't ideal.

Proposed solution

One possible approach to get around this that I found, is to create a new "parent class" for custom checks, i.e. Sa11yCustomChecks. This could be implemented like this:

// src/js/sa11y.js

// ...

class Sa11yCustomChecks {
  sa11y;

  setSa11y(sa11y) {
    this.sa11y = sa11y;
  }

  check() {}
}

// ...

export {
  Lang,
  Sa11yCustomChecks,
  Sa11y,
};

// -------------------------------------------------------------

// ./index.js
export { Sa11y, Lang, Sa11yCustomChecks } from './dist/js/sa11y.esm';
// ...

In this class, we handle setting the sa11y property, which means that custom check classes written by the user only need to include the check() method, as they extend Sa11yCustomClass like this:

// src/your-custom-checks.ts
import { Sa11yCustomChecks } from "sa11y";

export default class CustomChecks extends Sa11yCustomChecks {
  check() {
  // Add custom checks here.
  }
}

One could say this isn't quite solving the issue at it's core, only moving it to javascript which of course doesn't care about type definitions, but I don't think this should be a problem.

Let me know what you think!

[adamchaboryk]

Thanks for looking into this, Simon.

The CustomChecks implementation was not very ideal to begin with. I wish I broke up Sa11y into multiple files earlier on, then it would be a simple matter of importing the file.

Although passing class instances to each other on initialization was the only way to make Sa11y and the CustomChecks file "know" each other. I had help from the Joomla team with this, as previous attempts of mine was causing circle dependency issues.

If you can get your proposed solution working, please submit another PR.

Thanks!!

[smncd]

My pleasure!

The CustomChecks implementation was not very ideal to begin with. I wish I broke up Sa11y into multiple files earlier on, then it would be a simple matter of importing the file.

Gotcha, that makes sense. That's something that could be looked into for sure.

My proposal wouldn't change the way CustomChecks are implemented, but rather function as a kind of bridge to TS. Anyones existing CustomChecks would be completely unaffected, and the Sa11yCustomChecks class would only need to be used for Typescript.

I have it working locally, opening a pull request so you can inspect it: #35

[adamchaboryk]

Sounds great. Thanks again, @smncd