-
Couldn't load subscription status.
- Fork 39
feat: support configurable deployment basepath #158
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
feat: support configurable deployment basepath #158
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a typosquat?Package name is similar to other popular packages and may not be the package you want. Use care when consuming similarly named packages and ensure that you did not intend to consume a different package. Malicious packages often publish using similar names as existing popular packages. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
a809cb5 to
2f29274
Compare
2f29274 to
4a65858
Compare
4a65858 to
705a0f0
Compare
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment, @typescript-eslint/prefer-ts-expect-error -- only TS during build time will complain. | ||
| // @ts-ignore -- Rollup will fail to generate d.ts because it doesn't know import.meta.env. | ||
| const viteValue = import.meta.env | ||
| .VITE_VERCEL_OBSERVABILITY_BASEPATH as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also check for typeof import === 'undefined' like for projectId?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't have to, because import doesn't behave like process. process is a variable that belongs to the global namespace, while import is a keyword.
So it doesn't have a type, that's why I've wrapped it within a try-catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, tested with the provided usecases and default behaviour (without any ENV var).
Thanks for providing clear instructions! 🙌
a450c58 to
23388cb
Compare
🖖 What's in there?
Since ages, we're considering the possibility to configure, at build time, the basepath used for both downloading the injected script and reporting the collected metrics.
This PR brings such capability, and works with our supported frameworks: plain React (CRA), Next, Nuxt, Vue, Sveltkit, Astro, Remix (only when used with vite).
🤺 How to test?
Check the vercel deployments of each example app.
To reproduce locally, and set custom basepath:
pnpm i && pnpm -F @vercel/analytics buildThen:
For Nextjs app
NEXT_PUBLIC_VERCEL_OBSERVABILITY_BASEPATH="/abc" pnpm -F nextjs build pnpm -F nextjs startbrowse to http://localhost:3000 and check your console: it's trying (and failing) to download
/abc/insights/script.jsFor Nextjs 15 app
NEXT_PUBLIC_VERCEL_OBSERVABILITY_BASEPATH="/def" pnpm -F nextjs-15 build pnpm -F nextjs-15 startbrowse to http://localhost:3000 and check your console: it's trying (and failing) to download
/def/insights/script.jsFor Sveltekit app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/hij" pnpm -F sveltekit build pnpm -F sveltekit previewbrowse to http://localhost:4173 and check your console: it's trying (and failing) to download
/hij/insights/script.jsFor vue app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/klm" pnpm -F vue build pnpm -F vue previewbrowse to http://localhost:4173 and check your console: it's trying (and failing) to download
/klm/insights/script.jsFor nuxt app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/nop" pnpm -F nuxt build pnpm -F nuxt previewbrowse to http://localhost:3000 and check your console: it's trying (and failing) to download
/nop/insights/script.jsFor remix app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/qrs" pnpm -F remix build pnpm -F remix startbrowse to http://localhost:3000 and check your console: it's trying (and failing) to download
/qrs/insights/script.jsFor astro app
PUBLIC_VERCEL_OBSERVABILITY_BASEPATH="/tuv" pnpm -F astro build pnpm -F astro previewbrowse to http://localhost:4321 and check your console: it's trying (and failing) to download
/tuv/insights/script.jsThere is no example for CRA.
🔬Notes to reviewers
This is a generic solution, heavily inspired by @emspishak (thanks for the work!), and would replace #156.
I've changed the example app for Remix to used the vite, as per the latest official recommendation (remix 2.14 to date).
I've also replaced Jest with vitest, because the primer couldn't handle
import.metaproperly. And it's superior in my opinion (look at the reduced number of deps and shorter configuration).🔗 Related PRs