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(plugin-core): support minified url creation with pathname in assetUrl #1248

Merged

Conversation

maxkostow
Copy link
Contributor

Status

READY

Description

honeybadger-io/next suggest to set the assetUrl to https://my-app.vercel.app/_next (link)

However, the plugin-core uses the URL class in a way that does not support this. Instead of overwriting the assetsUrl pathname, this PR joins them.

Steps to Test or Reproduce

  1. Follow the instructions at https://docs.honeybadger.io/lib/javascript/integration/nextjs
  2. Observe that sourcemaps are not correctly uploaded including _next in the pathname

@subzero10 subzero10 self-requested a review November 16, 2023 20:33
@@ -81,7 +82,9 @@ export async function buildBodyForSourcemapUpload(
): Promise<FormData> {
const form = new FormData()

const minifiedUrl = new URL(sourcemapData.jsFilename, hbOptions.assetsUrl).href
const url = new URL(hbOptions.assetsUrl)
Copy link
Member

Choose a reason for hiding this comment

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

This is strange, the second parameter of URL() constructor is the baseUrl, so it shouldn't be replacing it.

I will test this on a nextjs project as well.

Copy link
Member

Choose a reason for hiding this comment

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

I confirm that the second parameter of the URL constructor will strip out any pathnames in the string.

@subzero10 subzero10 changed the title fix(plugin-core): support minified url creation with pathanme in asse… fix(plugin-core): support minified url creation with pathname in assetUrl Nov 23, 2023
Copy link
Member

@subzero10 subzero10 left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -81,7 +82,9 @@ export async function buildBodyForSourcemapUpload(
): Promise<FormData> {
const form = new FormData()

const minifiedUrl = new URL(sourcemapData.jsFilename, hbOptions.assetsUrl).href
const url = new URL(hbOptions.assetsUrl)
Copy link
Member

Choose a reason for hiding this comment

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

I confirm that the second parameter of the URL constructor will strip out any pathnames in the string.

@subzero10 subzero10 merged commit 8ba9626 into honeybadger-io:master Nov 23, 2023
@subzero10
Copy link
Member

This should be fixed with @honeybadger-io/nextjs@5.8.4!

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