Skip to content
This repository has been archived by the owner on Jan 21, 2023. It is now read-only.

feat: rewrite using Next.js api routes #162

Closed
wants to merge 5 commits into from

Conversation

nico-bachner
Copy link

Apologies in advance — this is a big one.

I completely rewrote the project using Next.js API Routes and also converted the homepage to use Next.

There most important thing to be aware of is that there are breaking changes. First, the endpoint is now at /api/[file], no longer at /[file]. Also, many of the query parameters have changed.
If these changes are inconvenient (backwards compatibility), I will be happy to review the PR and minimise the api changes.

Demo here: https://og-image-nine-nu.vercel.app

Additional note: this PR is not ready to merge yet because of an issue with puppeteer in production.

@vercel
Copy link

vercel bot commented May 28, 2021

@nico-bachner is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@nico-bachner nico-bachner marked this pull request as ready for review May 28, 2021 21:12
@nico-bachner nico-bachner requested a review from styfle as a code owner May 28, 2021 21:12
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

This looks really cool!

However, we can't accept breaking changes to the API at this time since its referenced by https://vercel.com/docs

@nico-bachner
Copy link
Author

Thanks for the PR!

This looks really cool!

However, we can't accept breaking changes to the API at this time since its referenced by https://vercel.com/docs

I thought that might be the case.

If you like, I can try and eliminate the breaking changes.

I can also offer another solution: since the api endpoint in this PR is at /api/[file] (as per Next.js conventions), and the endpoint currently is at /[file], it should be possible to have both coexist at the same time (this would give people the option between v1 and v2).

What do you think?

@styfle
Copy link
Member

styfle commented May 29, 2021

You can avoid the breaking change by adding rewrites to next.config.js

{
  rewrites: [{ source: '(.*)', destination: '/api']
}

@nico-bachner
Copy link
Author

It can be solved by adding “rewrites” to next.config.js

Which part of the issue do you mean by that?

@nico-bachner nico-bachner marked this pull request as draft June 16, 2021 20:51
@aguilaair
Copy link

aguilaair commented Sep 22, 2021

Which part of the issue do you mean by that?

Sorry for chiming is after such a long time, but I believe he means that by using rewrites you could make it backwards compatible as api path would be removed. This effectively redirects anything that fits the regex (.*) to /api behind the scenes.

@nico-bachner
Copy link
Author

Thanks @aguilaair for the explanation, but that part was clear to me. The breaking changes I am worried about are caused by my implementation being ever so slightly different from the original, which could potentially break existing users' URLs.

What I was suggesting to @styfle was that if we don't use redirects, then the two versions of the api could potentially coexist on the same domain, however after some experimentation, I realised it would be best to just stick to the original api.

@styfle
Copy link
Member

styfle commented Jan 20, 2023

Closing in favor of #226

@styfle styfle closed this Jan 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants