Skip to content

Conversation

@jdufresne
Copy link
Contributor

Fixes #1396

@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #1399 (9efedd2) into master (dfc863e) will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1399      +/-   ##
==========================================
- Coverage   88.13%   88.00%   -0.13%     
==========================================
  Files          29       29              
  Lines        1576     1576              
  Branches      221      221              
==========================================
- Hits         1389     1387       -2     
- Misses        139      140       +1     
- Partials       48       49       +1     
Impacted Files Coverage Δ
debug_toolbar/panels/profiling.py 89.28% <0.00%> (-1.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfc863e...9efedd2. Read the comment docs.

docs/changes.rst Outdated
now 2.2.

* JavaScript is now loaded as a `JavaScript module`_. When developing using the
Django development server, this will normally load fine and not present an
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct, CORS is required in any scenario where static files are served under a different domain, development or not.

Copy link
Contributor Author

@jdufresne jdufresne Nov 4, 2020

Choose a reason for hiding this comment

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

The Django development server is under one origin: localhost:8000. What you're describing is the 2nd part of the sentence "if your application server and static files server are at different locations".

Suggestions on wording to make this more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

JavaScript is now loaded as a `JavaScript module`_. If your application server
and static files server are at different origins (e.g. different host/port),
you may see CORS errors in your browser's development console. See the
"Cross-Origin Request Blocked" section of the :doc:`installation docs
<installation>` for details on how to resolve.

There's no need to mention development mode since it's not really relevant; only the origin is relevant.

Copy link
Contributor Author

@jdufresne jdufresne Nov 4, 2020

Choose a reason for hiding this comment

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

There is no mention of "development mode", perhaps this could be clearer. There is only a mention of the development server (aka runserver). I want to make clear to users that have a very standard environment -- e.g. the one outlined in the Django tutorial and docs -- that no action is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no mention of "development mode", perhaps this could be clearer.

It's not relevant at all. Why mention something that's not relevant? It doesn't matter if someone is using runserver, daphne, or anything else. It doesn't matter if they've followed the django tutorial, or have a production-ready setup.

The only factor that plays any part in this new requirements is if the origins are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is worthwhile to reassure users (especially new users) that are using a very typical setup that no additional action is required. Advanced users using an advanced setup are more likely to understand the nuances of different origins and CORS.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is worthwhile to reassure users (especially new users) that are using a very typical setup that no additional action is required. Advanced users using an advanced setup are more likely to understand the nuances of different origins and CORS.

Agreed, but I'm not sure the change log is read by new users. I think something closer to what @WhyNotHugo suggested would be better. Maybe we can highlight the happy flow case, then follow-up with the exception?

The Debug Toolbar loads a `JavaScript module`_.  Typical local 
machine development is not be impacted. However, if the
application and static files are served from different hosts...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, all. I've tried to clean this up a little from the provided feedback.

Cross-Origin Request Blocked
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The Debug Toolbar loads a `JavaScript module`_. When developing using the
Copy link
Contributor

Choose a reason for hiding this comment

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

When serving staticfiles under the same origin as the development server[...]

docs/changes.rst Outdated
now 2.2.

* JavaScript is now loaded as a `JavaScript module`_. Typical local development
using Django ``runserver`` is not be impacted. However, if your application
Copy link
Member

Choose a reason for hiding this comment

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

will not be impacted or is not impacted?

docs/changes.rst Outdated
server and static files server are at different origins, you may see CORS
errors in your browser's development console. See the "Cross-Origin Request
Blocked" section of the :doc:`installation docs <installation>` for details
on how to resolve.
Copy link
Member

Choose a reason for hiding this comment

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

"on how to resolve this issue"? (I'm not a native speaker so I don't know whether this would be an improvement.)

Cross-Origin Request Blocked
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The Debug Toolbar loads a `JavaScript module`_. Typical local development using
Copy link
Member

Choose a reason for hiding this comment

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

The wording is almost the same as above but not quite. Either the JavaScript loads a module or JavaScript is loaded as modules. Both may be correct (nitpicking, sorry)

@jdufresne
Copy link
Contributor Author

Thanks for the feedback @matthiask. I've applied your suggestion so take another look and let me know what you think. Please feel free to nitpick away, I think this is important to get right.

@auvipy auvipy merged commit b66d1c0 into django-commons:master Nov 13, 2020
@WhyNotHugo
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CORS required as of v3.0

5 participants