-
Notifications
You must be signed in to change notification settings - Fork 30
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
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 build
Then:
For Nextjs app
NEXT_PUBLIC_VERCEL_OBSERVABILITY_BASEPATH="/abc" pnpm -F nextjs build pnpm -F nextjs start
browse to http://localhost:3000 and check your console: it's trying (and failing) to download
/abc/insights/script.js
For Nextjs 15 app
NEXT_PUBLIC_VERCEL_OBSERVABILITY_BASEPATH="/def" pnpm -F nextjs-15 build pnpm -F nextjs-15 start
browse to http://localhost:3000 and check your console: it's trying (and failing) to download
/def/insights/script.js
For Sveltekit app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/hij" pnpm -F sveltekit build pnpm -F sveltekit preview
browse to http://localhost:4173 and check your console: it's trying (and failing) to download
/hij/insights/script.js
For vue app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/klm" pnpm -F vue build pnpm -F vue preview
browse to http://localhost:4173 and check your console: it's trying (and failing) to download
/klm/insights/script.js
For nuxt app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/nop" pnpm -F nuxt build pnpm -F nuxt preview
browse to http://localhost:3000 and check your console: it's trying (and failing) to download
/nop/insights/script.js
For remix app
VITE_VERCEL_OBSERVABILITY_BASEPATH="/qrs" pnpm -F remix build pnpm -F remix start
browse to http://localhost:3000 and check your console: it's trying (and failing) to download
/qrs/insights/script.js
For astro app
PUBLIC_VERCEL_OBSERVABILITY_BASEPATH="/tuv" pnpm -F astro build pnpm -F astro preview
browse to http://localhost:4321 and check your console: it's trying (and failing) to download
/tuv/insights/script.js
There 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.meta
properly. And it's superior in my opinion (look at the reduced number of deps and shorter configuration).🔗 Related PRs