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

STSMACOM-876 <EditableListForm> state.status initialize as null so it can pass null re-initialization checks on component update. #1540

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

BogdanDenis
Copy link
Contributor

@BogdanDenis BogdanDenis commented Nov 18, 2024

Description

Uncaught error in setError because state.status array is empty and we're trying to access an nth element when deleting a record has failed.
This is because status.empty is initialized in the constructor as [] but the re-initialization check in getDerivedStateFromProps checks for !state.status, so it's never again re-initialized (unless you add a new field which always re-initializes the state)

Screenshots

chrome_EvhYsh7s6H.mp4

Issues

STSMACOM-876

Copy link

github-actions bot commented Nov 18, 2024

Bigtest Unit Test Results

  1 files  ±0    1 suites  ±0   14s ⏱️ ±0s
508 tests ±0  471 ✅ ±0  37 💤 ±0  0 ❌ ±0 
510 runs  ±0  473 ✅ ±0  37 💤 ±0  0 ❌ ±0 

Results for commit 8560207. ± Comparison against base commit 0601825.

♻️ This comment has been updated with latest results.

… so it can pass `null` re-initialization checks on component update.
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

Oh, I hate these old, uncommented, heavily-lifecycle-dependent class components so much.

I understand your logic: if we initialize status to [] in the constructor, gDSFP will never rebuild it because [] isn't falsey. Fine. But, is initializing to null correct since we know it's supposed to be an array, or is there a better fix in gDSFP like if (isEmpty(this.status))?

I'm OK with this change. I just want to be sure it's the best/clearest change we can make that will resolve this bug.

@zburke
Copy link
Member

zburke commented Nov 18, 2024

PS, @BogdanDenis, please make sure to move the ticket to "awaiting release" when this merges. Technically, this should go into the CHANGELOG under 9.3.0 and merge to master, and then I'll cherry pick it onto the b9.2 release branch.

…n re-initialization checks on component update.
@BogdanDenis
Copy link
Contributor Author

@zburke That's a good suggestion to use isEmpty, I've updated the PR

Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

@BogdanDenis will this work correctly if we only include the gDSFP change?

@BogdanDenis
Copy link
Contributor Author

@BogdanDenis will this work correctly if we only include the gDSFP change?

It does, I've updated the PR

@BogdanDenis BogdanDenis merged commit 45615a8 into master Nov 25, 2024
15 checks passed
@BogdanDenis BogdanDenis deleted the STSMACOM-876 branch November 25, 2024 14:19
zburke pushed a commit that referenced this pull request Dec 2, 2024
… so it can pass `null` re-initialization checks on component update. (#1540)

## Description
Uncaught error in `setError` because `state.status` array is empty and
we're trying to access an nth element when deleting a record has failed.
This is because `status.empty` is initialized in the constructor as `[]`
but the re-initialization check in `getDerivedStateFromProps` checks for
`!state.status`, so it's never again re-initialized (unless you add a
new field which always re-initializes the state)

## Screenshots

https://github.com/user-attachments/assets/81511ca8-c338-43ad-ba41-a797134ff31c

## Issues
[STSMACOM-876](https://folio-org.atlassian.net/browse/STSMACOM-876)

(cherry picked from commit 45615a8)
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