Remove TemplateRendererProxy mock

[toasted-nutbread]

The need for a mock can be removed by just using dependency injection.

[github-actions]

✔️ No visual differences introduced by this PR.

View Playwright Report (note: open the "playwright-report" artifact)

[djahandarie]

Indeed, but if there is no usage of the dependency injection by the extension itself, wouldn't it be easy to understand (i.e., more intent-preserving) to use a mock here, even if it introduces extra lines of test code?

[toasted-nutbread]

Dependency injection is already used all over the place, even the japaneseUtil in the same constructor is doing it.

Further, the only reason why the TemplateRendererProxy abstraction exists is to allow it to run in a sandboxed window or some other context asynchronously. If no such functionality exists, there is no reason why a direct TemplateRenderer cannot be used instead. The APIs are intended to be equivalent, with the exception of async.

Even the async thing is only an unfortunate side effect of Handlebars not supporting async execution, which has had many unfortunate downstream effects and increase is code complexity, but that's not directly the issue here.

[djahandarie]

Dependency injection is already used all over the place, even the japaneseUtil in the same constructor is doing it.

Sure, I only meant to evaluate whether the dependency injection in this case was only being introduced to support testing as opposed to some functionality of the extension itself. Because if it only supports testing it would seem to me that would be clearer to use a mock (which puts the 'cognitive burden' of making sure the mock works on whoever writes the mock), as opposed to dependency injection (which puts the 'cognitive burden' on all modifiers of that part of the extension code that need to ensure their code will work with all the potential different valid dependencies that could be injected).

Further, the only reason why the TemplateRendererProxy abstraction exists is to allow it to run in a sandboxed window or some other context asynchronously. If no such functionality exists, there is no reason why a direct TemplateRenderer cannot be used instead. The APIs are intended to be equivalent, with the exception of async.

It sounds like you are saying there's a possibility this instance of dependency injection might be used by the extension itself? If so, then this seems fine to me.

[BTW, I'm not too picky about this so happy to approve regardless, just wanted to share the thought.]

[toasted-nutbread]

Yeah I get ya, it is kinda intended to primarily support this test at this point in time I suppose, but that could change in the future given all of the other changes that have happened related to the Handlebars stuff. If eval is no longer used, the sandbox window + proxy could potentially be removed, as that was the primary (only?) reason for needing to do that. So IMO it provides some future flexibility to more easily make such changes if the need or desire arises.

My other thought on the mocking stuff is that it introduces new code which has to be maintained. The way it's done after this update should involve less burden as it's more direct. vitest mocks are also apparently some weird form of black magic where they are hoisted and apply to the imports in the file, but don't apply if you are importing it as a reference, so personally I also find it be more immediately clear what exactly is happening in the test.

[djahandarie]

I think it'd be better for you to resolve the merge conflicts than me, so よろしく 🙇