-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added mime-types for UI files #19046
Conversation
dagster-io#18986 Graphs over a certain size were not rendering anymore, cause was a mime-type of 'text/plain' for js files. This fix adds explicit mime-types
Thanks for submitting a PR, @CamronBorealis! Can you provide more detail about which files are being sent with |
Hi @CamronBorealis what browser are you using and what version of starlette are you on? We haven't seen this issue before and we have various web workers running on the page so curious what's different about your setup. |
@hellendag @salazarm It appears that all .js files are being served with a Content-Type of User-Agent is
I'm not really familiar with web development these days, so please feel free to request any other information. If there's something obvious, it's possible I'm missing it. |
@CamronBorealis Are you on Windows? |
encode/starlette#829 (comment) This suggests the issue is a windows registry issue but quite a few libraries are opting to implement the workaround from this PR as a response, I think we should do the same, wdyt @hellendag ? |
@hellendag Yes, I am on Windows |
mimetypes.add_type('application/javascript', '.js') | ||
mimetypes.add_type('text/css', '.css') | ||
mimetypes.add_type('image/svg+xml', '.svg') |
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 like this works, but I think it may not be quite the right place for this, since this is where we're serving the index file.
Can you move these calls to build_static_routes
, so that they're alongside where we initialize the relevant static paths themselves?
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.
@hellendag Done! Is that better?
As requested by @hellendag
@CamronBorealis Sorry for the long delay! Let's try to get this thing landed. It looks like there's a build failure on Python formatting. https://buildkite.com/dagster/unit-tests/builds/336#018deba6-291f-43d4-bed7-77ad33a4b2d9/246-322. Can you run |
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.
Requesting changes for the formatting error.
I recreated the PR here so closing this one: https://github.com/dagster-io/dagster/pull/21129/files |
Recreates #19046 but with `make ruff` run --------- Co-authored-by: Camron B <camron.borealis@gmail.com>
Recreates #19046 but with `make ruff` run --------- Co-authored-by: Camron B <camron.borealis@gmail.com>
Summary & Motivation
#18986
Graphs over a certain size were not rendering anymore, cause was a mime-type of 'text/plain' for js files. This fix adds explicit mime-types for js, svg, and css files.
Credit to this post on Stackoverflow: https://stackoverflow.com/questions/60269909/is-there-a-way-to-explicitly-set-mime-types-for-starlette-uvicorn
How I Tested These Changes
I edited the dagster-webserver Python module in my environment directly, all graphs now render.