simple destroy #788
simple destroy #788
Conversation
Codecov Report
@@ Coverage Diff @@ ## develop #788 +/- ## =========================================== + Coverage 92.34% 92.43% +0.09% =========================================== Files 166 167 +1 Lines 40347 40290 -57 Branches 3719 3722 +3 =========================================== - Hits 37257 37241 -16 + Misses 3084 3043 -41 Partials 6 6
|
| | ||
| public destroy() { | ||
| if (this.gpu) { | ||
| this.gpu.destroy() |
There was a problem hiding this comment.
I'm not sure about removing this one. GPU.js docs say that we should run a proper cleanup: https://github.com/gpujs/gpu.js/#cleanup
There was a problem hiding this comment.
ok, this should be then delegated to the main-class destroy.
src/HyperFormula.ts Outdated
| for(const key of Object.keys(this)) { | ||
| delete (this as Record<string, any>)[key] | ||
| } | ||
| Object.setPrototypeOf(this, null) |
There was a problem hiding this comment.
This will end up with an error being thrown xxx is not a function if someone will try to call methods on a destroyed instance. It is a good solution but maybe HOT has a better example. As @budnix pointed we replace all the functions with postMortem method: https://github.com/handsontable/handsontable/blob/b0a09bde9a9d4164bee4f28cd047fa843f42a7a9/src/core.js#L4006-L4016
And the postMortem method displays a warning:
https://github.com/handsontable/handsontable/blob/b0a09bde9a9d4164bee4f28cd047fa843f42a7a9/src/core.js#L4043-L4047
| This pull request fixes 1 alert when merging b194edf into f411eba - view on LGTM.com fixed alerts:
|
Context
Simpler version of #786
How has this been tested?
Types of changes
Related issue(s):
Checklist: