-
Notifications
You must be signed in to change notification settings - Fork 30
fix: handle undefined comparators in Angular 20 #578
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
base: main
Are you sure you want to change the base?
fix: handle undefined comparators in Angular 20 #578
Conversation
- Add fallback to orderByComparator when column comparator is undefined - Prevent TypeError during sorting operations in Angular 20 reactive context - Add comprehensive tests for sorting with undefined comparators - Include test simulating Angular 20 reactive context with computed signals Fixes issue where cachedDir.compareFn is not a function during data updates
|
Patrick Kaster seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Summary of ChangesHello @PatrickKaster, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the @siemens/ngx-datatable library that caused sorting failures in Angular 20 applications. The issue stemmed from comparator functions becoming undefined during Angular's reactive signal recomputation, leading to runtime errors. The solution introduces resilient fallback logic to ensure that sorting operations always have a valid comparator, thereby restoring reliable data sorting behavior across reactive updates and maintaining full compatibility with previous Angular versions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively resolves a sorting issue encountered with Angular 20 by providing a fallback comparator. The changes are logical, and the addition of a new test file with specific scenarios, including reactive updates, is a great way to ensure the fix is robust and prevents future regressions. I have one suggestion regarding code clarity in sort.ts where a fallback appears to be redundant. Overall, this is a well-executed fix.
| ? (cachedDir.compareFn ?? orderByComparator)(propA, propB, rowA, rowB) | ||
| : -(cachedDir.compareFn ?? orderByComparator)(propA, propB, rowA, rowB); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback to orderByComparator here appears to be redundant. The cachedDir.compareFn is assigned its value on line 113, which is derived from the compareFn constant defined on line 108:
const compareFn = cols[dir.prop] ?? orderByComparator;This initial assignment already ensures that cachedDir.compareFn will hold a valid function, defaulting to orderByComparator if the column-specific comparator is missing. Consequently, the nullish coalescing operators (??) in this block are not necessary. Removing them would improve code clarity by avoiding the implication that cachedDir.compareFn could be nullish at this stage.
| ? (cachedDir.compareFn ?? orderByComparator)(propA, propB, rowA, rowB) | |
| : -(cachedDir.compareFn ?? orderByComparator)(propA, propB, rowA, rowB); | |
| ? cachedDir.compareFn(propA, propB, rowA, rowB) | |
| : -cachedDir.compareFn(propA, propB, rowA, rowB); |
|
THX a lot for your contribution @PatrickKaster. I am not able to reproduce your issue. Can you please create a reproduction here https://stackblitz.com/fork/github/siemens/ngx-datatable/tree/main/projects/stackblitz and share the link. In general, the |
|
Hi @spike-rabbit, Thank you for looking into this. I completely understand the hesitation since it isn't reproducible in an isolated StackBlitz. To be transparent: I am also unsure if the root cause lies entirely within the library or if it's an edge-case interaction with how our application handles async state updates via STOMP in Angular 20. Our runtime logs confirm that How we mitigated this: Regarding this PR: If you prefer to keep the library strict, I am happy to close this. However, if you see value in this guard for the sake of overall stability, the PR is available for consideration. Thanks again for your time and for maintaining this project! |
|
@PatrickKaster I hope we will find the root cause for this behavior. In your application, do you set the |
What kind of change does this PR introduce?
What is the current behavior?
When using
@siemens/ngx-datatablewith Angular 20, sorting fails after data updates.During Angular’s reactive signal recomputation, the library’s internal sort cache (
cachedDir) may contain an undefined comparator function, causing:This occurs inside
sortRows()when Angular 20 recomputes internal rows viacomputed(), which can cause comparator references to be lost.Affected versions:
Sorting and table updates break after CRUD operations.
What is the new behavior?
A safe fallback is added so the comparator function is always valid:
orderByComparatorif a column’s comparator is undefined:This prevents comparator loss during Angular 20’s signal recomputation and ensures that sorting works correctly after data updates.
Additional improvements:
Tests added include:
Does this PR introduce a breaking change?
Other information
cachedDir.compareFn is not a functionduring sorting in Angular 20