-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
ref(ui): Consolidate server frontend hydration logic #13868
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
ref(ui): Consolidate server frontend hydration logic #13868
Conversation
6062740
to
c8215d9
Compare
@@ -0,0 +1,151 @@ | |||
import 'bootstrap/js/alert'; |
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.
This is just a mv
of index.js
.
@@ -1,153 +1,15 @@ | |||
import '@babel/polyfill'; | |||
import 'bootstrap/js/alert'; |
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.
Might be easier to just view this as it's own new file.
whitelistUrls: {% convert_to_json ALLOWED_HOSTS %} | ||
}; | ||
window.__SENTRY__USER = {% get_user_context request %} || undefined; | ||
</script> |
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.
Removed since this is inlined. This file was overriden in getsentry, but see the PR there for how it's been changed.
|
||
@register.simple_tag | ||
def public_dsn(): | ||
return get_public_dsn() |
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.
File removed because it was only used in that one sentry-sdk.html template.
@@ -27,11 +27,9 @@ def get_default_context(request, existing_context=None, team=None): | |||
from sentry.plugins import plugins | |||
|
|||
context = { | |||
'CSRF_COOKIE_NAME': settings.CSRF_COOKIE_NAME, |
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 longer needed in templates.
'URL_PREFIX': options.get('system.url-prefix'), | ||
'SINGLE_ORGANIZATION': settings.SENTRY_SINGLE_ORGANIZATION, | ||
'PLUGINS': plugins, | ||
'ALLOWED_HOSTS': list(settings.ALLOWED_HOSTS), |
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 longer needed in templates.
callback(module); | ||
window.csrfCookieName = csrfCookieName; | ||
window.__sentryGlobalStaticPrefix = distPrefix; | ||
window.__SENTRY__OPTIONS = sentryConfig; |
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.
Do we still need these globals?
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.
Maybe only the cookie one here: https://github.com/getsentry/sentry/blob/master/src/sentry/static/sentry/app/constants/index.jsx#L102
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.
But I would rather refactor them in another PR I think
</script> | ||
|
||
{% block scripts %} | ||
{% include "sentry/includes/sdk-config.html" %} |
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.
If this include is being removed why do you update the file in getsentry?
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.
We still use sdk-config
in getsentry to override the __initialData.sentryConfig
object. I'm sure I could come up with a better way to specify this configuration for sentry, which we probably will want to come up with a solution for. But for now I'm okay with this solution.
- Consolidates all frontend hydration data into a single global window variable `__initialData`. - Splits application bootstraping into `bootstrap.jsx`, to allow for initial data setup before bootstrapping the application.
593f86e
to
55b0c93
Compare
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.
Looks good to me. The change that broke last time has been fixed in the getsentry changes.
* master: chore: Fix typo errywhere -> everywhere (#13934) ref: (Django 1.9) Bump djangorestframework to 3.0.5 as an intermediate step to get to 3.3.x feat(issueless events): Test eventstream work without groups (#13888) ref: Improve repr of User (#13896) feat(loader): Make the default for new js projects v5 of js sdk (#13327) build: Remove browser-reload flag (#13918) ref(ui): Consolidate server frontend hydration logic (#13868) ref: Refactor user reports to not use Postgres Event (#13904) Test/integration acceptance tests (#13895) feat: Remove ts-jest (#13846) obs(sentry_apps): Add a small metrics increment on processing resource changes. (#13897) enable testing for all orgs (#13903) ref: Update Python SDK to 0.10.0 (#13911) fix: Do not suggest Sentry.Extensions.Logging when ASP.NET Core is used (#13891) Update license year to 2019 ci(travis): Move docker-sentry builds to Dockes Hub autobuild (#13901) ref(app-platform): Add integration features to SentryAppDetailsModal (#13393) chore(ui) Rename OrganizationStream to IssueList (#13892)
Consolidates all frontend hydration data into a single global window variable
__initialData
.Splits application bootstraping into
bootstrap.jsx
, to allow for initial data setup before bootstrapping the application.This also requires https://github.com/getsentry/getsentry/pull/3018 to correctly override the
__initialData.sentryConfig
object for getsentry production. Without this, we'll have the same issue as when this was deployed from #13828, where the DSN, environment, and release will not be correctly set.