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 empty query param when calling external dependency toolbar.js #1456

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lucasra1
Copy link

@lucasra1 lucasra1 commented Oct 7, 2024

Changes

Changed the URL to use t=<num> as query param instead of empty/undefined param name (v=161.1&=<num>).

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Backstory

We use posthog in a nextjs application and use nextjs's rewrites as a proxy for better tracking. We noticed that the toolbar does not work on our site. when checking the logs we noticed that the rewrites did not work for the toolbar:

image

 code: 'ERR_INVALID_URL',
  input: 'https__ESC_COLON_//eu-assets.i.posthog.com/static/__ESC_COLON_path*'

But the web-vitals.js loaded without a problem...

image

Further investigation lead to the empty query param for the toolbar.js rotating token to bust the cdn cache.

?v=1.166.1?&=1728316200000

The current stable version of next.js (14.2.14) has a "bug" that can't handle empty query params correctly in rewrites. The most recent canary version of nextjs seems to not have this bug anymore.

But still I think its better when posthog does not use an empty query param as this seems like something that was not intentional in the first place and secondly can result in unexpected behavior in frameworks like nextjs

Copy link

vercel bot commented Oct 7, 2024

@lucasra1 is attempting to deploy a commit to the PostHog Team on Vercel.

A member of the Team first needs to authorize it.

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@lucasra1
Copy link
Author

Please keep open and review. Thanks

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.

2 participants