refactor: centralize into functions #377
Conversation
| * @returns {Promise<void | never>} | ||
| */ | ||
| async function runElmReviewInWatchMode() { | ||
| async function runElmReviewInWatchMode(options, errorHandler) { |
There was a problem hiding this comment.
I keep meaning to circle back to this. Unrelated to this change, the recursive nature of this function means I think it leaks memory and could potentially stack-overflow. Or just have bad stacktraces.
Related to this diff, because references to options & errorHandler now need to passed down and stored on the stack of every recursion, I think this change effectively ⅓s the number of saves elm-review can run it watch mode before crashing. It should still be an absurdly large number, but could be a potential issue?
Then again, I could have just misread the code or perhaps v8 does more optimization to recursion than I think it does.
There was a problem hiding this comment.
Copilot informed me that asynchronous recursion doesn’t stack-overflow (i.e., callbacks that are in new event loop ticks have new stacks), so that’s not an issue, but passing in options might leak memory.
StackOverflow agrees, so putting that as LARGE WARNING against merging this.
There was a problem hiding this comment.
Alright, I won't merge the PR in the current state then. The other changes (look good though to be clear).
Maybe we should just revisit this function's implementation entirely (I can't right now though, upcoming vacation), though that's probably easier said than done, I haven't looked at the function in a long time.
There was a problem hiding this comment.
Ok, did some more investigation, and I think it's fine... not really sure how to double-check that though.
lishaduck
force-pushed
the
the-big-refactor-pt-3
branch
2 times, most recently
from
17ff53a to
8a02c2c
Compare
lishaduck
force-pushed
the
the-big-refactor-pt-3
branch
2 times, most recently
from
c90da54 to
23f2a87
Compare
lishaduck
force-pushed
the
the-big-refactor-pt-3
branch
from
23f2a87 to
e1cfd62
Compare
2 participants
Stop running stuff on module evaluation, instead run it on executing
main().Depends on #380.