edrlab / thorium-reader

A cross platform desktop reading app, based on the Readium Desktop toolkit
https://www.edrlab.org/software/thorium-reader/
BSD 3-Clause "New" or "Revised" License
1.86k stars 157 forks source link

Fixes #2602 (Electron BrowserWindow WebContents LifeCycle Sanity Check) #2640

Closed danielweck closed 2 weeks ago

danielweck commented 2 weeks ago

Hello @panaC in the code diff you will see:

1) the obvious isDestroyed code fencing / sanity checks 2) yield take(winActions.library|reader.openSucess.build) replaced with yield take(winActions.library|reader.openSucess.ID) (I can't believe this passed compilation! no idea what the runtime behaviour was/is??) 3) "did-finish-load" ALWAYS used, not just in DEV mode otherwise there can be differences with the production builds which we do not test as much as the development builds. Consequently I commented the yield put(winActions.library|reader.openSucess... immediately after the loadUrl, and also to improve code legibility I positioned the "did-finish-load" event handler BEFORE the loadUrl (consistent for both reader and library windows)

panaC commented 2 weeks ago

take support the toString with StringableActionCreator

https://github.com/edrlab/thorium-reader/blob/9d2e3e840425b9d35292f036167148c0f1812f9f/src/main/redux/actions/win/reader/openSucess.ts#L30

https://github.com/redux-saga/redux-saga/blob/9ec63efbffb576a5d2c8b4bbf9df8f020693fdad/packages/types/types/index.d.ts#L22

https://github.com/redux-saga/redux-saga/blob/9ec63efbffb576a5d2c8b4bbf9df8f020693fdad/packages/types/types/index.d.ts#L35

danielweck commented 2 weeks ago

ah thanks! phew! :)

that being said, this fact is obfuscated and inconsistent with the many other places in Thorium's codebase where take() uses the ID directly instead of via build().toString() ...let's do a quick code scan to see where else this is used.

panaC commented 2 weeks ago

ah thanks! phew! :)

that being said, this fact is obfuscated and inconsistent with the many other places in Thorium's codebase where take() uses the ID directly instead of via build().toString() ...let's do a quick code scan to see where else this is used.

One day we will switch to the latest rtk api ! :) I hope :)

panaC commented 2 weeks ago

I don't think it's a good choice to force use .ID instead of .build

we lost type checking for the takeEveryTyped function. image

I think we have to revert this commit https://github.com/edrlab/thorium-reader/pull/2640/commits/dd38b5c1241edfb5f8f83e612834d3e2d9de0533

Our type checking api is not ready to use .ID only

danielweck commented 2 weeks ago

ah, I verified "npm run build:prod" before merging! do you get this error in the develop branch??

panaC commented 2 weeks ago

I made this error, to prove action type checking is no longer checked/inferred

danielweck commented 2 weeks ago

I see. In the meantime I was looking at the codebase again, it would be better to be consistent the other way around: only use .build, not .ID :)

danielweck commented 1 week ago

Follow-up: https://github.com/edrlab/thorium-reader/commit/79a62f3184fc66eb39a38fc6dd98c42e3f51903f

and https://github.com/edrlab/thorium-reader/commit/3eef469fe71592c5c590aca2386b7e654bb36222