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.

Sign up for GitHub

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

Jump to bottom

fix(d.ts): RunOptions.reporter can be any string #4218

Merged
merged 2 commits into from
Nov 2, 2023
Merged

fix(d.ts): RunOptions.reporter can be any string #4218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77ee9bf
fix(d.ts): RunOptions.reporter can be any string
WilcoFiers Oct 25, 2023
7c9616f
Merge branch 'develop' into type-issue-reporter
straker Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions axe.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ declare namespace axe {

type TagValue = string;

type ReporterVersion = 'v1' | 'v2' | 'raw' | 'raw-env' | 'no-passes';
type ReporterVersion = 'v1' | 'v2' | 'raw' | 'rawEnv' | 'no-passes';

type RunOnlyType = 'rule' | 'rules' | 'tag' | 'tags';

Expand Down Expand Up @@ -132,7 +132,7 @@ declare namespace axe {
interface RunOptions {
runOnly?: RunOnly | TagValue[] | string[] | string;
rules?: RuleObject;
reporter?: ReporterVersion;
reporter?: ReporterVersion | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this adds any value in typescript. The resolved type of this is just string and it looks like it overrides any value of the ReporterVersion. Might just make this string and not have the set values.

screenshot showing that the resolved type of a set list of strings OR string is just string screenshot of the resolved type when using a variable of type that is a set list of strings OR string is just string

Copy link
Contributor Author

@WilcoFiers WilcoFiers Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, but can we do better? I looked at using a generic for this, something like:

interface RunOptions <T extends string | void = void>{
    reporter?: ReporterVersion | T;
}

That's fine, but then you can't pass this into axe.run because it expects RunOptions<void>. To fix that you'd have to make the reporter something that is passed into axe.run, and to AxeResults (for toolOptions), and AxeReporter, and getFrameContexts... It's a lot. I don't think that's worth the complexity. If someone want to be sure their reporter type is valid they can still check it with reporterVersion. Open to suggestions though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at a GitHub-wide code search for "axe.addReporter", there are exactly zero folks who are actually using axe.addReporter in practice in an open source project (all hits are copies of axe-core or its tests).

Based on that, I definitely don't think making this more accurate is worth making the entire RunOptions type much more complex, and I don't think it's worth making the intellisense worse (like steve's screenshots point out) either; my inclination is to optimize for folks that aren't using addReporter and actually just do this:

  /** If you've added a custom ReporterVersion with `axe.addReporter`, that is also valid here */
  reporter?: ReporterVersion

In practice, I think if we're improving this type, the more important improvement would be to add the existing AxeReporter function as an allowable value.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I can also live with ReporterVersion | string if Wilco feels strongly about it - I only feel strongly that we shouldn't pollute the whole RunOptions type with a generic over this)

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 do prefer ReporterVersion | string here.

resultTypes?: resultGroups[];
selectors?: boolean;
ancestry?: boolean;
Expand Down
8 changes: 7 additions & 1 deletion typings/axe-core/axe-core-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import * as axe from '../../axe';

var context: any = document;
var $fixture = [document];
var options = { iframes: false, selectors: false, elementRef: false };
var options: axe.RunOptions = {
iframes: false,
selectors: false,
elementRef: false
};
options.reporter = 'rawEnv';
options.reporter = 'custom';

axe.run(context, {}, (error: Error, results: axe.AxeResults) => {
if (error) {
Expand Down
Loading