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

fix(common/web): replace invalid StrsItem identity checks with isEqual() #13122

Conversation

markcsinclair
Copy link
Contributor

@markcsinclair markcsinclair commented Feb 3, 2025

In PR #12868 a new isEqual() mathod was added to StrsItem to distinguish identity (===) and equality (toEqual()) checks. This was needed to get correct behaviour from ElemElement.isEqual() and ElementString.isEqual() which both (now) depend on StrsItem.isEqual().

This PR checks uses of StrsItem to identify where StrsItem.toEqual() should be used instead of an identity check.

Files examined:

  • element-string.ts
  • kmx-plus.ts
  • string-list.ts
  • build-keys.ts
  • build-layr.ts
  • build-strs.ts
  • build-uset.ts

Fixes: #12945

@keymanapp-test-bot skip

@markcsinclair markcsinclair marked this pull request as draft February 3, 2025 12:48
@markcsinclair
Copy link
Contributor Author

markcsinclair commented Feb 3, 2025

In the end, no instances were found where identity was incorrectly used instead of the (new) StrsItem.toEqual(). In all cases, it seems that direct string comparison (StrsItem.value) was made rather than checks on the StrsItem. The only occasions where a bug was found were in the ElemElement.isEqual() and ElementString.isEqual(), corrected in earlier PR #12868.

This PR should still be merged, however, as it cointains some new unit tests.

@markcsinclair markcsinclair marked this pull request as ready for review February 3, 2025 15:41
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

I think it's worth doing the review, and the test, as isEqual would actually be the best way to compare.

It's entirely possible that a StrsItem will be used with isEqual in the future.

@markcsinclair markcsinclair merged commit 6ead43b into master Feb 4, 2025
21 checks passed
@markcsinclair markcsinclair deleted the fix/common/web/types/12945-replace-invalid-StrsItem-identity-checks-with-isEqual branch February 4, 2025 11:20
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.184-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bug(common/web): potential identity/equality bugs with StrsItem
4 participants