React - deleting array elements stored in state deletes wrong element

Question

I've been beating my head against this for long enough that I'm not sure I'd even see an obvious problem.

The App has rows, and each row has cells of varying sizes. Adding rows works. Adding cells works. Deleting rows however, does not. If I click the delete button for row 0, row 2 disappears. If I click the button for row 1, row 2 disappears. Looking at the react dev tools, it appears that under the hood the right row is being deleted (in state), the right row is selected for deletion, but no matter what it doesn't render the way I would expect (if I hit delete for row 0, row 0 disappears, for example).

If I delete from the highest row down it works, but if I start at the zeroth row it won't let itself be deleted.

deleteRowButtonHandler of the App class is where I've been focusing my efforts.

deleteRowButtonHandler( e )
        {
            const targetRow = parseInt( e.target.id ) 
console.log( "TARGETING: ", targetRow )

            const currentRows = this.state.rows

            let newArray = []
            currentRows.forEach( entry =>
            {
                console.log( "Looking at ", entry.id )
                if( entry.id == targetRow )
                    console.log( 'skipping entry with id', entry.id )
                else
                    newArray.push( entry )
            })

            console.log( newArray )
            this.setState( {rows: newArray})
            //let newRows = currentRows.filter( thisRow => {console.log( thisRow.id + ' and the target ' +  targetRow);
             //   if( thisRow.id == targetRow )
             //   {
             //       console.log( thisRow )
             //       console.log( '...is the row that we are trying to delete' )
            //    }

            //    return thisRow.id !== targetRow} 
            //) 
            //this.setState( {rows: newArray}, console.log( "NEW STATE:", this.state ))
        }

I've tried a few variations (filter, splice, doing it in a few lines, doing it in a lot of lines, etc) so I'm not particularly proud of that chunk of code at the moment.

https://codepen.io/philipvdg/pen/mdmmWLG is the code; any pointers in the right direction would be greatly appreciated.

Update Speaking generally, the problem was that I (being a newbie) didn't understand that the constructor was only run once. In the constructor of a child component I had this.cells = this.props.cells and then, in the render method, called this.cells.map( ... ). Since the constructor only ever ran once, this.cells was never updated after a state change.

Which version of the code are you asking about? The uncommented or commented one? Only include one version of code or show two different code snippets if you are showing two different attempts. Each code snippet should be such that we can copy/paste and run ourselves without any changes (such as uncommenting). — Code-Apprentice
– Code-Apprentice, Commented Jul 16, 2021 at 19:42
While you are debugging, be aware that the second parameter to this.setState() should be a callable. In the commented out version console.log( "NEW STATE:", this.state ) is called immediately and its return value is passed to this.setState(). Instead, you probably want something like newState => console.log( "NEW STATE:", newState). — Code-Apprentice
– Code-Apprentice, Commented Jul 16, 2021 at 19:44
@Code-Apprentice Yes, I'm aware of that, thank you. In that particular case those two lines are vestigial remnants from various other attempts at getting this to work. I've already learned the value of the setState callback. Thanks! — Philip R
– Philip R, Commented Jul 17, 2021 at 23:54

Aleksandr Smyshliaev · Accepted Answer · 2021-07-18 15:12:52Z

1

It get duplicate ids somehow.

Looking at  3
    pen.js:74 Looking at  4
    pen.js:81 (4) [{…}, {…}, {…}, {…}]
    pen.js:225 TEMPLATE PROPS:  {rows: Array(4)}rows: Array(4)0: {cells: Array(0), id: 0}1: {cells: Array(1), id: 3}2: {cells: Array(0), id: 3}3: {cells: Array(0), id: 4}length: 4__proto__: Array(0)__proto__: Object
    pen.js:232 row 0 is {cells: Array(0), id: 0}
    pen.js:232 row 1 is {cells: Array(1), id: 3}
    pen.js:232 row 2 is {cells: Array(0), id: 3}
    pen.js:232 row 3 is {cells: Array(0), id: 4}

// in newRowButtonHandler
this.setState( { rows: [ ...this.state.rows, {cells:[], id: this.state.rows.length} ] } )

// try this
    this.setState( { rows: [ ...this.state.rows, {cells:[], id: Math.round(Math.random() * 1000000)} ] }) )

UPD Problem with id's, render and delete function use different set of values. https://codepen.io/Asmyshlyaev177/pen/yLbXBGo?editors=0010

Should not use e.target.id, should be single source of truth e.g. react state.

edited Jul 18, 2021 at 15:12

answered Jul 16, 2021 at 21:02

Aleksandr Smyshliaev

4284 silver badges10 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

Philip R Over a year ago

That's certainly something I'll change once I get to the bottom of the current problem, thanks for pointing it out. In the pre-loaded data (the rows that are hard-coded at the top of the code) there aren't duplicate IDs so I don't think that's directly related to the problem.

Aleksandr Smyshliaev Over a year ago

Working version, problem with how you use id's codepen.io/Asmyshlyaev177/pen/yLbXBGo?editors=0010

Philip R Over a year ago

If you try to delete the zeroth row, it still doesn't get removed though. With the default data, click the delete button on row zero and the row with the three col-xs-1 cells doesn't go anywhere. I don't get what's wrong with using e.target.id - that just contains the id of the delete button which contains the row to delete. Could you expand on why that's a bad idea?

Aleksandr Smyshliaev Over a year ago

You should have 1 place to store state, when you spread state between React and html attributes it much harder to track and easier to make mistakes, e.g. need to write some code to synchronize state in 2 places.

Aleksandr Smyshliaev Over a year ago

It didn't get removed because you assign cells from props to variable, but constructor function run only on first render.

|

Konstantin Modin · Accepted Answer · 2021-07-16 19:53:23Z

1

In Controls render you should add line const id = row.id; so it will be like this:

this.props.rows.map( 
   ( row ) => {
     const id = row.id; 
    let rowId = 'controlRow' + id

Using array idx is wrong. You should use idx from data structure.

answered Jul 16, 2021 at 19:53

Konstantin Modin

1,3439 silver badges12 bronze badges

3 Comments

Philip R Over a year ago

"Wrong" as in it won't work or is it something that's just generally frowned upon for some reason?

Philip R Over a year ago

Interesting, I've never heard of the array ID being buggy - is that a known/documented bug in Javascript or something specific to React?

Konstantin Modin Over a year ago

You can read this article: robinpokorny.medium.com/…

Collectives™ on Stack Overflow

React - deleting array elements stored in state deletes wrong element

2 Answers 2

7 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

7 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related