Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Render decorator for Koa Driver #1482

Merged
merged 7 commits into from
Feb 14, 2025

Conversation

Telokis
Copy link
Contributor

@Telokis Telokis commented Jan 21, 2025

Description

Fixes the @Render decorator when using Koa by hijacking the next function to ensure the ctx.render promise will be completed.

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

fixes #1147, fixes #902, fixes #610, fixes #602

@attilaorosz
Copy link
Member

Unfortunately I’m not familiar with koa so I cannot review this.
@NoNameProvided any suggestions?

@Telokis
Copy link
Contributor Author

Telokis commented Jan 28, 2025

Unfortunately I’m not familiar with koa so I cannot review this. @NoNameProvided any suggestions?

@attilaorosz Hey, thanks for replying!

I can give more details:

  • First of all, the previous code (this.koa.use) was very wrong and should absolutely be removed even if it's not replaced. It registers a new middleware that will always call ctx.render, it's not for the current route or handling but forever. As soon as one @Render route is called, the middleware is added and subsequently used.
  • The next function of Koa is a Promise. Our ctx.render also is a promise that must be await-ed but the current function is not async. In order not to alter the flow too much, I use a trick to redefine the next function (that will eventually be await-ed) so it runs the ctx.render first.

I've already implemented this solution on my local version to confirm it works and the tests are also there to provide some insurance about it.

@Telokis
Copy link
Contributor Author

Telokis commented Feb 6, 2025

@attilaorosz @NoNameProvided Hello!

Can I get some feedback on this? It's a bit annoying to manually edit the file everytime I reinstall my modules so the feature can work as expected! haha

Thanks!

@attilaorosz
Copy link
Member

attilaorosz commented Feb 14, 2025

Sorry for the late response!
While I generally agree with fixing this, my main concern is that while the old koa-views seemed to support any kind of templating engine, koa-ejs only supports ejs. I'm not sure if anyone is using anything else. Might be a good idea to provide a bridge between routing-controllers and whatever templating engine instead.

@Telokis
Copy link
Contributor Author

Telokis commented Feb 14, 2025

Sorry for the late response! While I generally agree with fixing this, my main concern is that while the old koa-views seemed to support any kind of templating engine, koa-ejs only supports ejs. I'm not sure if anyone is using anything else. Might be a good idea to provide a bridge between routing-controllers and whatever templating engine instead.

@attilaorosz Hello and thanks for coming back to my PR!
About the templating engine, I haven't selected any specific one, I just randomly picked ejs because I know it used to be popular and I needed one for testing.
My implementation only expects a ctx.render function to be defined and in my personal project using routing-controller, I manually define it myself like so:

	koaApp.context.render = async function (view: string, locals?: object) {
		this.body = await nunjucksRenderPromise(view, locals);
		this.status = 200;
		return this;
	};

Note: I've fixed the formatting.

@Telokis
Copy link
Contributor Author

Telokis commented Feb 14, 2025

koa-views can still be used. (The new one because the old one is deprecated)
routing-controller didn't make any choice regarding this, anyway. With or without my PR, no engine is present or configured by default.

Without my PR, though, using @Render at all will break the app as soon as the decorated controller is visited.

@attilaorosz
Copy link
Member

attilaorosz commented Feb 14, 2025

Sounds good. Sorry, as I mentioned I'm not experienced with koa. I'm open to do this but then let's remove the koa-views dev dependency.

@Telokis
Copy link
Contributor Author

Telokis commented Feb 14, 2025

Sounds good. Sorry, as I mentioned I'm not experienced with koa. I'm open to do this but then let's remove the koa-views dev dependency.

@attilaorosz No worries, it's fine! I'll remove the dev dependency, indeed I missed it!

Edit: It's done!

@attilaorosz attilaorosz merged commit 77eafac into typestack:develop Feb 14, 2025
5 checks passed
@attilaorosz
Copy link
Member

Thanks for the contribution!

@Telokis
Copy link
Contributor Author

Telokis commented Feb 18, 2025

Thanks for the contribution!

@attilaorosz No problem, thanks for merging! Do you think it will be released soon?

@attilaorosz
Copy link
Member

Releasing right now

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants