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

feat: Add Spotlight #60523

Merged
merged 12 commits into from
Dec 7, 2023
Merged

feat: Add Spotlight #60523

merged 12 commits into from
Dec 7, 2023

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Nov 23, 2023

Adds Spotlight to local Sentry

Google.Chrome.mp4

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Nov 23, 2023
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

@HazAT HazAT marked this pull request as ready for review November 28, 2023 19:12
@HazAT HazAT requested review from a team as code owners November 28, 2023 19:12
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Merging #60523 (40e1fa7) into master (927112b) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #60523      +/-   ##
==========================================
- Coverage   81.08%   81.08%   -0.01%     
==========================================
  Files        5186     5188       +2     
  Lines      227811   227974     +163     
  Branches    38221    38247      +26     
==========================================
+ Hits       184726   184847     +121     
- Misses      37459    37502      +43     
+ Partials     5626     5625       -1     
Files Coverage Δ
src/sentry/conf/server.py 89.91% <ø> (ø)
src/sentry/utils/sdk.py 71.17% <ø> (ø)
static/app/bootstrap/initializeSdk.tsx 0.00% <ø> (ø)

... and 74 files with indirect coverage changes

static/app/bootstrap/initializeSdk.tsx Outdated Show resolved Hide resolved
webpack.config.ts Outdated Show resolved Hide resolved
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
@HazAT
Copy link
Member Author

HazAT commented Nov 28, 2023

I still can find spotlight in the prod build cc @AbhiPrasad :(

webpack.config.ts Outdated Show resolved Hide resolved
@AbhiPrasad
Copy link
Member

I still can find spotlight in the prod build cc @AbhiPrasad :(

Yeah this makes sense actually because of the sentryConfig.environment conditional, it can't be evaluated at bundle time, so even if sentryConfig.environment is marked as side effect free it'll be kept.

you'll have to do

if (process.env.NODE_ENV !== 'production') {
  if (sentryConfig.environment === 'development) {
    Spotlight.init();
  }
}

yarn.lock Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
Copy link
Member

@scttcper scttcper left a comment

Choose a reason for hiding this comment

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

if postcss, tailwind, etc are only used to build the package they should be moved to devDependencies and react 18 should be a peer dependency. if possible react-router-dom should be removed as well since we're not on v6

yarn.lock Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@dcramer
Copy link
Member

dcramer commented Nov 28, 2023

Before we merge lets answer why we need ANY deps to load Spotlight.

My expectation is its ENTIRELY isolated, just like the CSS is, and should have zero conflicts.

Its ok to load a 2mb local JS package...

@HazAT
Copy link
Member Author

HazAT commented Nov 29, 2023

@scttcper Thank you! The dependency problem has been resolved
That was critical for Spotlight

@HazAT
Copy link
Member Author

HazAT commented Nov 29, 2023

I still can find spotlight in the prod build cc @AbhiPrasad :(

Yeah this makes sense actually because of the sentryConfig.environment conditional, it can't be evaluated at bundle time, so even if sentryConfig.environment is marked as side effect free it'll be kept.

you'll have to do

if (process.env.NODE_ENV !== 'production') {
  if (sentryConfig.environment === 'development) {
    Spotlight.init();
  }
}

@AbhiPrasad this worked!

Copy link
Member

@scttcper scttcper left a comment

Choose a reason for hiding this comment

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

lookin good 👍

@HazAT HazAT merged commit dba826f into master Dec 7, 2023
52 of 53 checks passed
@HazAT HazAT deleted the feat/spotlight branch December 7, 2023 13:13
@asottile-sentry
Copy link
Member

asottile-sentry commented Dec 7, 2023

reverting: broke the build

yarn run v1.22.5
$ /workspace/node_modules/.bin/tsc -p config/tsconfig.build.json
static/app/bootstrap/initializeSdk.tsx(5,28): error TS2307: Cannot find module '@spotlightjs/spotlight' or its corresponding type declarations.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@asottile-sentry asottile-sentry added the Trigger: Revert add to a merged PR to revert it (skips CI) label Dec 7, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 40da94a

getsentry-bot added a commit that referenced this pull request Dec 7, 2023
This reverts commit dba826f.

Co-authored-by: asottile-sentry <103459774+asottile-sentry@users.noreply.github.com>
@HazAT HazAT restored the feat/spotlight branch December 7, 2023 15:21
@HazAT HazAT mentioned this pull request Dec 7, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants