fix dynamic callback patcher: make things more versatile
- Dominant language
- JavaScript
- Stars
- 171
- Forks
- 13
- PR merge metrics
- No merged PRs in 30d
Description
* [x] use `Proxy` instead of `function`, so property modification also gets taken care of correctly.
* [ ] add analysis for multi-patching of same function/callback
* → We have started adding that info to `GlobalView` → `Patched_Callbacks`
* [ ] ideally: don't nest patched functions/callbacks under any circumstances
* (e.g. we might get: `patchedCallback` -> `patchedThenCb`)
* NOTE: `patchedFunctions` are instrumented un-instrumented functions while `patchedCbs` are instrumented recorded functions. The two sets should never overlap.
* [ ] test: `javascript-algorithms` errors out
* might be caused by `jest-snapshot`
* [x] test: `require.resolve` should never be patched
## Remaining Problems
Some problems are still generally unsolved (and require some hacky workaround):
* Consumer of a function relies on equality checks (e.g. `addEventListener` vs. `removeEventListener`)
* [ ] add custom fix for `removeEventListener` and similar
E.g., using our current proxy solution, `f` does not retain identity when handled by uninstrumented code:
```js
var s = new Set();
function f() {}
var p1 = new Proxy(f, {});
var p2 = new Proxy(f, {});
s.add(p1);
console.assert(s.has(p2)); // assertion fails
```
## More Future Work
* [ ] determine heuristics to detect and avoid "instrumenting the instrumentors"
* Generally speaking, it is a bad idea to instrument the instrumenters, since that can lead to infinite loops and other bugs.
* E.g. the old problem of `graceful-fs` instrumenting `process.cwd` → which in Node@14 was used internally by Node when reading sourcemaps when generating stacktraces → which triggered Dbux recording
## Done
* [x] fix: `react-tic-tac-toe` does not render if `CallbackPatcher` is `Enabled`.
* [x] fix `addEventListener` -> `removeEventListener` callback identity
Contributor guide
Assessment
This issue has not been assessed yet.