Skip to content

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

Conversation

evanpurkhiser
Copy link
Member

@evanpurkhiser evanpurkhiser commented Jul 1, 2019

  • 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.

@evanpurkhiser evanpurkhiser requested review from markstory, mattrobenolt and a team July 1, 2019 20:55
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/refui-consolidate-server-frontend-hydration-logic branch from 6062740 to c8215d9 Compare July 1, 2019 21:01
@evanpurkhiser evanpurkhiser requested a review from billyvg July 1, 2019 22:22
@@ -0,0 +1,151 @@
import 'bootstrap/js/alert';
Copy link
Member Author

@evanpurkhiser evanpurkhiser Jul 1, 2019

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';
Copy link
Member Author

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>
Copy link
Member Author

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()
Copy link
Member Author

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,
Copy link
Member Author

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),
Copy link
Member Author

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;
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

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" %}
Copy link
Member

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?

Copy link
Member Author

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.
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/refui-consolidate-server-frontend-hydration-logic branch from 593f86e to 55b0c93 Compare July 2, 2019 18:21
Copy link
Member

@markstory markstory left a 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.

@evanpurkhiser evanpurkhiser merged commit ded5372 into master Jul 8, 2019
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/refui-consolidate-server-frontend-hydration-logic branch July 8, 2019 22:19
jan-auer added a commit that referenced this pull request Jul 9, 2019
* 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)
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants