PostHog / posthog-js

Send usage data from your web app or site to PostHog, with autocapture.
https://posthog.com/docs/libraries/js
Other
266 stars 111 forks source link

Feature Request: Optional trailing slash #814

Closed yvesbuschor closed 5 months ago

yvesbuschor commented 11 months ago

Situation

The Posthog API works with trailing slashes (e.g. /decide/?v=3 and not /decide?v=3). This is hardcoded in the posthog-js library which generally makes sense.

Problem

For people working with a reverse proxy (e.g. with Vercel) the endpoint that gets called still has that trailing slash. Next.js and Vercel by default redirect to the endpoint without trailing slash (this could be overwritten but only for the whole site - see Next.js docs). This causes every request to the reverse proxy to first trigger a redirect with 308. posthog-redirect

While the 308 can't be avoided without changing the global trailing slash config, the reverse proxy can still redirected correctly by re-adding the slash. For example a working reverse proxy for Next.js with default config:

    "rewrites": [
      {
        "source": "/ingest/:match*",
        "destination": "https://eu.posthog.com/:match*/"
      }
    ]

Proposed Feature

In the posthog.init function add a config option trailing_slash (type: Boolean, default: true). If set to false, requests wouldn't include the trailing slash. Thus not triggering a 308 in the situation described above.

Open Questions

I think not all endpoints work without trailing slash. (e.g. we had issues when loading the site-app for survey beta). This means disabling trailing slash should only be possible when there's a reverse proxy set up like above adding the trailing slash back again.

So I'm not sure if this is something that should actually be part of the library or just a documented limitation. Without the change people using Next.js without trailing slashes won't be able to use the rewrite function for reverse proxy or have to accept the redirect loops.

(Happy to create a PR but would first like to hear some opinions)

fabianuribe commented 9 months ago

I am also observing the redirects happening for all Posthog requests when using the reverse proxy approach with Next.JS, due to Next.JS default handling of trailing slashes.

I think @yvesbuschor proposal to expose a config would be definitely be welcomed, since it would halve the number of requests to the proxy. Although to be fair, it is a Next.JS specific issue and I am not sure whether Posthog should be concerned or would event want work around other framework's limitations. For what is worth, my solution was to strip the trailing slash at the edge using Cloudflare's URL rewrite rules . So far, I don't observe any meaningful latency increase due to the rewrites and it effectively takes care of all those unnecessary redirects and effectively reduced ~30% of our posthog's aggregate request times.

Without rewrites

Screenshot 2023-12-09 at 5 46 47 PM

With rewrites

Screenshot 2023-12-09 at 5 49 27 PM
viantirreau commented 8 months ago

Hi @fabianuribe!

Could you share more details about your solution using Cloudflare URL Rewrite rules? I think I could manage to do it with the regex_replace function, but apparently, I need a Business plan or a WAF Advanced plan to be able to use that function.

Thanks in advance!

fabianuribe commented 8 months ago

Hi @fabianuribe!

Could you share more details about your solution using Cloudflare URL Rewrite rules? I think I could manage to do it with the regex_replace function, but apparently, I need a Business plan or a WAF Advanced plan to be able to use that function.

Thanks in advance!

Hey @viantirreau , I believe you get 10 rewrite rules even on Free Plan.

Here is what my rule configs look like, I hope it helps:

Under: Rules > Transform Rules > Rewrite Rules

Screenshot 2024-01-10 at 11 52 31 PM Screenshot 2024-01-10 at 11 52 59 PM
viantirreau commented 8 months ago

Brilliant! Works like a charm. Thank you @fabianuribe!

robmarshall commented 5 months ago

Some additional context. It is not possible to use this in Capacitor JS as a 308 redirect is seen as a CORs issue. The app is already in existence without trailing slashes. We would need to change all the endpoints for this to work.

mustafa421 commented 5 months ago

Following. Is there any other workaround for those not using Cloudflare or Vercel?

florianjuengermann commented 5 months ago

With nextjs you can now disable the trailingbackslash redirect:

// next.config.js
module.exports = {
  skipTrailingSlashRedirect: true,
}

https://nextjs.org/docs/app/building-your-application/routing/middleware#advanced-middleware-flags

robmarshall commented 5 months ago

@florianjuengermann Did this work for you? I tried it with no success.

florianjuengermann commented 5 months ago

@robmarshall Yes I have a setup with nextjs & vercel that works now. What is your issue?

My config:

// next.config.js
module.exports = {

 skipTrailingSlashRedirect: true,
  async rewrites() {
    return [
      {
        source: "/ingest/:path(.*)",
        destination: "https://us.i.posthog.com/:path",
      },
      {
        source: "/ingest/static/:path(.*)",
        destination: "https://us-assets.i.posthog.com/static/:path",
      },
    ];
  },
}
// posthog_provider.ts
posthog.init(process.env.NEXT_PUBLIC_POSTHOG_KEY!, {
  api_host: "/ingest",
  ui_host: "https://app.posthog.com",
});
zlwaterfield commented 5 months ago

With nextjs you can now disable the trailingbackslash redirect:

// next.config.js
module.exports = {
  skipTrailingSlashRedirect: true,
}

nextjs.org/docs/app/building-your-application/routing/middleware#advanced-middleware-flags

Incredible find, thank you @florianjuengermann! I just tested for rewrites, API routes and middleware, and it works for all three.

I am going to update the docs today with this.

zlwaterfield commented 5 months ago

I'm going to close this issue because this looks to have a solution now.