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

Give React SPA frontend specific things a home #907

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Oct 15, 2024

Replaces #904

We're moving away from the REACT frontend so we must make it easier to switch between the old and the new. One day we'll delete all the REACT frontend specific things, we ought to make it safe and easy.

This means settings-wrangling.

App settings come in two varieties:

  • App specific settings, often with a prefix
  • Standard django settings that the app assumes is set in a certain way

Much have been written about the best way to tackle 1. Not much about 2.

Furthermore standard django settings are of two types: Overridable (like DEBUG) and should be mutated in place (MIDDLEWARE, TEMPLATES..) The trend is towards more complex settings like the latter.

Finally, there's what to do about the root urls.py.

@hmpf hmpf requested review from a team October 15, 2024 10:52
@hmpf hmpf self-assigned this Oct 15, 2024
Copy link

github-actions bot commented Oct 15, 2024

Test results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit c50013f.

♻️ This comment has been updated with latest results.

@hmpf hmpf added API Affects Argus' REST API API v2 Ideas for API v2, backwards incompatible OK auth Touches the authentication/authorization subsystem frontend Has/needs companion issue in frontend help wanted Extra attention is needed labels Oct 15, 2024
@hmpf hmpf marked this pull request as draft October 15, 2024 10:54
Copy link
Collaborator

@elfjes elfjes left a comment

Choose a reason for hiding this comment

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

I like were this is going, good stuff.

one comment below

if not CORS_ALLOWED_ORIGINS:
raise ImproperlyConfigured('"FRONTEND_URL" has not been set in production!')

COOKIE_DOMAIN = get_str_env("ARGUS_COOKIE_DOMAIN", required=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still going to be required if we're not doing anything spa? And same question for ARGUS_FRONTEND_URL I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

frontend url might be otherwise useful but cookie domain is spa only. I'll rename 'em I think, to make it clearer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ARGUS_FRONTEND_URL is only for CORS, right? I don't think we should have any cors if we don't have a spa?

@hmpf hmpf force-pushed the spa-frontend-specific branch 4 times, most recently from b5ca8fc to bdc4900 Compare October 16, 2024 09:53
Copy link

sonarcloud bot commented Oct 16, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API v2 Ideas for API v2, backwards incompatible OK API Affects Argus' REST API auth Touches the authentication/authorization subsystem frontend Has/needs companion issue in frontend help wanted Extra attention is needed
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

2 participants