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

Remove dequal from the project #554

Merged
merged 2 commits into from
Jul 4, 2024
Merged

Remove dequal from the project #554

merged 2 commits into from
Jul 4, 2024

Conversation

samccone
Copy link
Contributor

@samccone samccone commented Jul 4, 2024

These two changes combined remove the need to pull in the dependency to the project.

This PR is inspired by the work done in A11yance/axobject-query#357 and A11yance/axobject-query#354

As an interesting finding when walking through the code to remove this dependency I discovered(in 431ad28) that the prior code-path utilizing dequal was impossible (always false situation)

Screenshot 2024-07-04 at 9 58 19 AM

Specifically we were doing a comparison between

type ElementARIARoleRelationTuple = [ARIARoleRelationConcept, RoleSet]

and

type ARIARoleRelationConcept = {|
....
|};

Which given the definition of a flow typed union means that these two types have NO overlap.

Previously the code was comparing
ElementARIARoleRelationTuple to a ARIARoleRelationConcept which would always be false, flatten the code based on this finding.
LCOV similarly also shows that this codepath was never utilized.
Copy link

codesandbox-ci bot commented Jul 4, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@ljharb
Copy link
Collaborator

ljharb commented Jul 4, 2024

Are we confident the types are correct tho?

@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 9797843892

Details

  • 18 of 23 (78.26%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 94.798%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/elementRoleMap.js 18 23 78.26%
Totals Coverage Status
Change from base Build 5366160847: -0.7%
Covered Lines: 257
Relevant Lines: 263

💛 - Coveralls

@samccone
Copy link
Contributor Author

samccone commented Jul 4, 2024

Are we confident the types are correct tho?

Great question - my ladder of logic was

  1. Since this function is not part of the userland API / interface of the library and rather is used to build up the internal mapping state the type should be knowable by inspecting the call sites
  2. The code IS expressed (at least the surrounding functions) by test coverage
  3. if hard-coded to find no matches is the internal state the same (YES)
  4. if i reverse the equality check do things break (YES)

From this I have a reasonable level of confidence as to the correctness of the flow typing

@ljharb
Copy link
Collaborator

ljharb commented Jul 4, 2024

That makes sense - is there a chance that this is masking a bug, and it should be executing that code? If so, deleting it seems worse than trying to figure it out, or leaving it there.

package.json Outdated Show resolved Hide resolved
src/elementRoleMap.js Outdated Show resolved Hide resolved
@samccone
Copy link
Contributor Author

samccone commented Jul 4, 2024

That makes sense - is there a chance that this is masking a bug, and it should be executing that code? If so, deleting it seems worse than trying to figure it out, or leaving it there.

I have root caused the defect however I need guidance from the project owner on what to do with the failures as a result of the long-simmering defect introducing drift in the fixtures file that is part of the tests
#555 (comment)

For now my recommendation would be to land this change, and we can use the other PR to reintroduce the correct code (as right now the logic in place is not only misleading but totally unused)

Unroll the equality check inside of the file for the specifc type comparision.
@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 9799139177

Details

  • 18 of 23 (78.26%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 94.798%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/elementRoleMap.js 18 23 78.26%
Totals Coverage Status
Change from base Build 5366160847: -0.7%
Covered Lines: 257
Relevant Lines: 263

💛 - Coveralls

@ljharb ljharb merged commit f5b8f4c into A11yance:main Jul 4, 2024
7 checks passed
samccone added a commit to samccone/aria-query that referenced this pull request Jul 7, 2024
As discovered in A11yance#554

Address the defect introduced f7f6120#r143856081

This change depends on A11yance#558 which when applied (as in this PR makes the teset cases pass)
ljharb pushed a commit to samccone/aria-query that referenced this pull request Jul 8, 2024
As discovered in A11yance#554

Address the defect introduced f7f6120#r143856081

This change depends on A11yance#558 which when applied (as in this PR makes the teset cases pass)
ljharb pushed a commit to samccone/aria-query that referenced this pull request Jul 8, 2024
As discovered in A11yance#554

Address the defect introduced f7f6120#r143856081

This change depends on A11yance#558 which when applied (as in this PR makes the teset cases pass)
ljharb pushed a commit to samccone/aria-query that referenced this pull request Jul 8, 2024
As discovered in A11yance#554

Address the defect introduced f7f6120#r143856081

This change depends on A11yance#558 which when applied (as in this PR makes the teset cases pass)
@benmccann benmccann mentioned this pull request Sep 4, 2024
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.

4 participants