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.

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

feat(axe.d.ts): add nodeSerializer typings #4551

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

hamax
Copy link
Contributor

@hamax hamax commented Aug 1, 2024

This API was added in pr #4093, but TS definitions were never added.

For simplicity I'm using SerialDqElement in the API. We could introduce a generic for the custom serialized type (T extends SerialDqElement), but it's hard to consistently use it everywhere (AxeReporter, NodeSerializer.dqElmToSpec).

I also fixed DqElement.mergeSpecs which is needed to implement a custom node serializer: it exists on the constructor and not on the individual instances

@hamax hamax requested a review from a team as a code owner August 1, 2024 14:57
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Sorry for the late response and thanks for putting this together. I have a minor suggestion.

axe.d.ts Outdated
@@ -405,6 +408,27 @@ declare namespace axe {
boundingClientRect: DOMRect;
}

interface CustomNodeSerializer {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be more useful to have this be a type parameter and have all functions take and return that type. It would ensure that the functions take and return the same thing.

interface CustomNodeSerializer<T = SerialDqElement> {
  toSpec: (dqElm: DqElement) => T
  mergeSpecs(nodeSpec: T, parentFrameSpec: T) => T
}

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 also needed to make the nodeSerializaer.update generic to be able to take any CustomNodeSerializer (if it only takes CustomNodeSerializer then making CustomNodeSerializer generic wouldn't really change anything.

There are still a lot of other functions that are not properly using this new type T here, like dqElmToSpec, RawNodeResult etc. Without changing the API, a lot of these functions would need to be return only generics, which is not great.

The way we use this is our code is by making our custom type extend SerialDqElement and only add optional additional properties which is not perfect but works around the type issue.

@hamax
Copy link
Contributor Author

hamax commented Aug 27, 2024

@straker Hey, can you take another look? See inline comment above.

@straker
Copy link
Contributor

straker commented Aug 29, 2024

Apologies, didn't see the reply you gave earlier. I'm currently working on fixing our broken unit tests and then I can take another look at this.

Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Thanks for your patience and for the contribution!

@dbjorge dbjorge dismissed straker’s stale review September 24, 2024 16:33

discussed with straker

@dbjorge
Copy link
Contributor

dbjorge commented Sep 24, 2024

Approved for security and validated formatting

@dbjorge dbjorge merged commit a2f3a48 into dequelabs:develop Sep 24, 2024
19 of 21 checks passed
@dbjorge dbjorge changed the title feat(axe.d.ts): add nodeSerializer API feat(axe.d.ts): add nodeSerializer typings Sep 24, 2024
@hamax hamax deleted the node-serializer branch October 3, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants