Skip to content

Conversation

@cleptric
Copy link
Member

@cleptric cleptric commented Feb 8, 2023

Screenshot_2023-03-06_at_11 33 34

This is a little PoC to offer the installation of one of our JavaScript SDKs when running php artisan sentry:publish.

closes #646

@cleptric cleptric requested a review from stayallive February 13, 2023 13:20
@cleptric cleptric marked this pull request as ready for review February 14, 2023 11:22
Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Only thing really is maybe to use import.meta.env.SENTRY_LARAVEL_DSN instead of hardcoding the DSN, otherwise this looks like it's more than sufficient!

This might be good enough because you would only use this in new packages, but another options would be to add a // @TODO: Read DSN from environment instead of hardcoded or something?

@cleptric
Copy link
Member Author

import.meta.env.SENTRY_LARAVEL_DSN

I think we need to prefix them with VITE_.

To prevent accidentally leaking env variables to the client, only variables prefixed with VITE_ are exposed to your Vite-processed code. e.g. for the following env variables:

https://vitejs.dev/guide/env-and-mode.html#env-files

Let's target Laravel 10/9 with Vite, as 8 and below use Webpack. I don't see many people creating new projects with these older versions.

I'll also add another "Vanilla.js" choice to install our default browser integration.

Additionally, we can also expand the snippets, as all Laravel version ship with Axios by default, so we can also add performance monitoring.

import * as Sentry from "@sentry/browser";
import { BrowserTracing } from "@sentry/tracing";

Sentry.init({
  dsn: import.meta.env.VITE_APP_SENTRY_LARAVEL_DSN,
  integrations: [new BrowserTracing()],
  tracesSampleRate: 1.0,
});

Brb!

@stayallive
Copy link
Collaborator

Ah right, the prefix, yes! We also need to publish that env variable then in addition to the regular DSN env variable, we can just add VITE_SENTRY_LARAVEL_DSN="${SENTRY_LARAVEL_DSN}" though to expose it with an alias instead of duplicating it.

@cleptric cleptric requested a review from stayallive March 6, 2023 10:32
@stayallive stayallive merged commit bd90979 into master Mar 7, 2023
@stayallive stayallive deleted the install-js-sdk branch March 7, 2023 10:25
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.

Install an JavaScript SDK during sentry:publish

5 participants