shaack / cm-chessboard

A JavaScript chessboard without dependencies. Rendered in SVG, coded in ES6. Views FEN, handles move input, animated, responsive, expandable
MIT License
216 stars 68 forks source link

Error occurs when chessboard is constructed and destroyed immediately afterwards #47

Closed Nandakumar-M closed 3 years ago

Nandakumar-M commented 3 years ago

I am using cm-chessboard on my Vue app. Noticed an interesting issue when my Vue chessboard component (which internally makes use of cm-chessboard) is mounted and unmounted immediately afterwards.

Uncaught (in promise) TypeError: Cannot read property 'inputWhiteEnabled' of undefined at eval (ChessboardView.js?8d80:434)

eval @ ChessboardView.js?8d80:434 Promise.then (async)
setCursor @ ChessboardView.js?8d80:433 eval @ ChessboardView.js?8d80:150 setTimeout (async)
eval @ ChessboardView.js?8d80:146 redraw @ ChessboardView.js?8d80:144 eval @ Chessboard.js?8ff1:93 setTimeout (async)
eval @ Chessboard.js?8ff1:92 ChessboardView @ ChessboardView.js?8d80:48 eval @ Chessboard.js?8ff1:84 Chessboard @ Chessboard.js?8ff1:83 mounted @ ProblemViewer.vue?ca1b:63

From what I could see, for responsive chessboards, the ChessboardView constructor makes a call to handleResize function which in turn ends up calling setCursor.

if (chessboard.props.responsive) {
            setTimeout(() => {
                this.handleResize()
            })
        }

Because of the setTimeout, this ends up getting called after the chessboard has been destroyed.. I tried removing the setTimeout and called the handleResize function synchronously from the ChessboardView constructor.. This seems to solve the issue for me.

I am wondering if the setTimeout is there for any specific reason.

The constructor in ChessBoard.js also has a similar setTimeout block.

Regards, Nanda

shaack commented 3 years ago

I honestly don't know anymore exactly why the timeouts have been there. I removed them both (https://github.com/shaack/cm-chessboard/commit/815edccffdfe7efe13b869e32bf7f7498de0bad1), the one in the view and the one in the constructor and everything works fine. Please try version 3.12.9, it should work better with Vue now. Would be nice if you give me feedback again, if it works now.

Nandakumar-M commented 3 years ago

Unfortunately, I am getting the same error even with the newer version. While it seemed to work yesterday, today it doesn't.

On checking again, there seems to be several things that could probably cause this..

I can see that the setCursor and destroy methods both rely on the promise initialized. Looks like the latter gets called first after the initalized promise resolves.

Also, there are a bunch of debounces (using setTimeout) and I am not sure why these are needed. I can understand the rationale for resizeDebounce but not sure why drawPiecesDebounce, redrawDebounce, drawMarkersDebounce are required. Would be great if you could please clarify this.

Finally, a possible solution could be that chessboard objects can have a field is_destroyed which gets set when destroy function is called. All API methods can be made no-op methods on destroyed chessboards using this field.

All that said, the issue does not affect my app in any way other than the error logged in the console. Nice of you to spend time working even on these minor issues. Thanks.

Regards, Nanda

shaack commented 3 years ago

The idea of the debounces is, if someone sets multiple pieces or multiple markers, like so:

addMarker("e4", typeXY);addMarker("e5", typeXY);addMarker("e6", typeXY);addMarker("e7", typeXY);
// or
setPiece("e4", king);setPiece("e5", pawn);... 30 more ....

the redraw will only be done once after all pieces or markers are added. Without debounce the board would remove and redraw all pieces/markers multiple times (on every call). It's just for performance. Additional a setTimeout makes the drawing async, so the process does not have to wait for all drawing to finish.

Regardless, I think I have fixed the problem with 3.12.10. If not, please reopen the ticket.