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(perf): Use @sentry/status-page-list for domain status link #68899

Merged
merged 2 commits into from Apr 15, 2024

Conversation

gggritso
Copy link
Member

@gggritso gggritso commented Apr 15, 2024

e.g.,
Screenshot 2024-04-15 at 10 43 28 AM

Use @sentry/domain-status-list, which is where we're going to keep a domain --> status page lookup. Load the library async when the component is rendered so we don't increase everyone's bundle. Right now it's about 14KB.

  • Add package
  • Use status-page-list library

Async load `status-page-list` instead of using our own hard-coded
constant.
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 15, 2024
Copy link

codecov bot commented Apr 15, 2024

Bundle Report

Changes will increase total bundle size by 17.6kB ⬆️

Bundle name Size Change
sentry-webpack-bundle-array-push 26.33MB 17.6kB ⬆️

@gggritso gggritso marked this pull request as ready for review April 15, 2024 14:57
@gggritso gggritso requested review from a team as code owners April 15, 2024 14:57
const [mod, setMod] = useState<any>({});

const loader = useCallback(async () => {
const loaded = await import('@sentry/status-page-list');
Copy link
Member

Choose a reason for hiding this comment

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

seems excessive to load what an object with 50 things in it, assuming we can tree shake the rest of it. We might need this getsentry/status-page-list#6

Performance page routes are already lazy loaded.

Copy link
Member

Choose a reason for hiding this comment

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

what i'm saying is it might be fine to just import {domainToStatusPageUrls} from '@sentry/status-page-list'

Copy link
Member Author

Choose a reason for hiding this comment

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

@scttcper I'm happy to follow whatever you think is best!

The docs in status-page-list suggested to load async, which seems sound since that package is going to grow and we probably can't tree-shake it, since we don't know which domains we need ahead of time, right?

Copy link
Member

Choose a reason for hiding this comment

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

yeah was thinking mostly of shaking everything else

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, roger! Will update the dependency if/when getsentry/status-page-list#6 is merged, then

@gggritso gggritso merged commit 65d5cd3 into master Apr 15, 2024
43 checks passed
@gggritso gggritso deleted the feat/perf/http-module-more-domain-status branch April 15, 2024 21:06
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants