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

Add comments count endpoint #198

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

typeofweb
Copy link
Contributor

@typeofweb typeofweb commented Oct 19, 2021

A custom API route for getting comment count. Useful for more advanced use-cases such as fetching & displaying the number of comments below each blogpost.

Obviously, this is just a draft because there's plenty of repetitive code to pages/api/discussions/index.ts.

Fixes #145

@vercel
Copy link

vercel bot commented Oct 19, 2021

@mmiszy is attempting to deploy a commit to the giscus Team on Vercel.

A member of the Team first needs to authorize it.

@laymonage
Copy link
Member

Hey, thanks for the PR.

However, before you continue working on this, I want to give a heads-up: I might not merge this. The reason is, well... this API would basically just be a proxy to GitHub's API. I'm trying to keep such endpoints to a minimum, to minimize the chance of giscus getting rate limited (it has happened before, see #188), or exceeding my Vercel monthly limits.

I mean, if GitHub provides a way to access the API without authentication (like the REST API), I would never even have pages/api/discussions/index.ts in the first place (at least the GET handler).

I do acknowledge that this endpoint could be useful, especially because the GraphQL API isn't very friendly. Maybe we could host such APIs separately (e.g. on Cloudflare Workers), or make this opt-in for those who self-host giscus...

@typeofweb
Copy link
Contributor Author

Yes, I understand the issue. I think the solution might be to only enable it for self-hosted instances or (in the future) for paid accounts?

@laymonage
Copy link
Member

laymonage commented Oct 20, 2021

Yes, I understand the issue. I think the solution might be to only enable it for self-hosted instances

Yup, likely through an environment variable that's enabled/disabled by default (I don't mind which one), so I can configure it from my Vercel dashboard.

Next.js does not have a built-in functionality to exclude certain pages from the production build, though. We probably have to do it manually, for example with the following variables:

ADDITIONAL_PAGES=api/discussions/comments-count.tsx,api/some-other-api.ts
ENABLE_ADDITIONAL_PAGES=false

And then, write something in the build script so that if ENABLE_ADDITIONAL_PAGES is false, all the files specified in ADDITIONAL_PAGES will be removed before the build.

or (in the future) for paid accounts?

I'm planning to keep it free for all users.

@AntoineSoetewey
Copy link
Contributor

I'm planning to keep it free for all users.

Thanks!!

@typeofweb
Copy link
Contributor Author

I don't see a way to do that currently

ADDITIONAL_PAGES=api/discussions/comments-count.tsx,api/some-other-api.ts
ENABLE_ADDITIONAL_PAGES=false

However, it would be trivial to add an if (!process.env.ENABLE_ADDITIONAL_PAGES) { return res.status(404).end(); }

@laymonage
Copy link
Member

However, it would be trivial to add an if (!process.env.ENABLE_ADDITIONAL_PAGES) { return res.status(404).end(); }

Yeah, but that would still create the page and its serverless functions though? 🤔

Vercel might cache it, but I don't know for sure.

@typeofweb
Copy link
Contributor Author

Yes, it will create the endpoint. Is that a problem @laymonage ?

@laymonage
Copy link
Member

@mmiszy I don't know how Vercel deals with it, but theoretically we will still deploy the page and the function will still be called every time someone hits the endpoint. The function would instantly finish, but if Vercel doesn't cache the response and counts it towards the usage, it can add up.

@laymonage
Copy link
Member

This might be useful: https://nextjs.org/docs/api-reference/next.config.js/custom-page-extensions

We can rename existing pages to have .page.tsx extension and name these optional pages with an extension such as .page.opt.tsx and add page.opt.tsx to the pageExtensions config based on process.env.ENABLE_ADDITIONAL_PAGES.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get the number of comments outside of the blog page
3 participants