Open ball-hayden opened 2 years ago
Thanks @ball-hayden, you definitely raised some interesting points. I need to dive deep into this to have a significant input on that subject. I'll start review this later today 🙂
@ball-hayden That's very impressive. You brought up a lot of questions here, I left a few comments with thoughts back. I'll continue the review next week.
Note that I'm not very proficient in this code territory, since I wasn't involved at any point in writing it, as you can see there isn't much documentation or logical separation into commits.. this is definitely a weak spot on this massive (and impressive) project (DetoxSync) 😅
Anyway I'll try my best to help in this investigation, another note that it might take me some time to answer as we are both under-capacity and the fact that it takes time to get into things 🙂
Thanks @asafkorem.
For the avoidance of doubt, I'm not expecting to merge this PR - it's entirely for discussion and ideas.
As I mentioned in https://github.com/wix/Detox/issues/3207#issuecomment-1102524857 I'm afraid I'm also having to leave this alone in work time - there isn't the business appetite to spend more time on it at the moment.
I'll continue to try and keep some thoughts out of hours though - it's an interesting problem and I'd love to know what the root cause actually is.
@ball-hayden thanks again for this investigation and the refactoring suggestions! 🚀
I believe some of the fixes here can be extracted into smaller PRs. If you'd like to do so, I would love to start merging some of the changes, like this one: https://github.com/wix/DetoxSync/pull/33/commits/716e2a7ad0c8f3ad5027aac9c3d276f4b93f1965 🙂
Have you tried running all the changes with detox, specifically with our tests suite?
Various thoughts while investigating https://github.com/wix/Detox/issues/3207
Best reviewed commit-by-commit.
Thoughts and explanations are left as comments inline.