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

Dynamic properties mimetype fix #1594

Merged
merged 6 commits into from
Sep 18, 2023

Conversation

thetableman
Copy link
Contributor

This PR aims to resolve the Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "text/plain". error with elements using dynamic_properties.js, examples of this error are noted in #1510 and #1579.

Credit to @natankeddem for finding and suggesting the fix in #1579.

@thetableman
Copy link
Contributor Author

thetableman commented Sep 10, 2023

Tested the additional mimetype in __init__.py of the main nicegui directory and in ui.py however the issue persisted. Applying to element.py resolved the issue for me for both echart and aggrid elements.

Given this error seems to affect only certain systems additional testers would be welcome.

@natankeddem
Copy link
Contributor

If you can take a quick look in regedit, could you double check this is what you are seeing?

Windows Registry Editor Version 5.00

[HKEY_CLASSES_ROOT\.js]
"Content Type"="text/plain"

If so I can "break" my PC similarly to test this out. Are you running directly in Windows 10 or using WSL?

@thetableman
Copy link
Contributor Author

I have the issue when using Win 11 directly (no WSL). Confirming that "Content Type"="text/plain" for .js.

@rodja rodja linked an issue Sep 11, 2023 that may be closed by this pull request
@rodja
Copy link
Member

rodja commented Sep 18, 2023

This is originally a Windows/Starlette issue which other projects also have build workarounds for: encode/starlette#829

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @thetableman!

I have two questions:

  1. Can we move the lines into nicegui.py close to static_files = StaticFiles(...). I think this is the place that actually requires the fix.
  2. I think the correct mime type for JavaScript would be "text/javascript" (see https://stackoverflow.com/a/21098951/3419103). Or am I missing something?

@falkoschindler falkoschindler added this to the 1.3.14 milestone Sep 18, 2023
@natankeddem
Copy link
Contributor

2. I think the correct mime type for JavaScript would be "text/javascript" (see https://stackoverflow.com/a/21098951/3419103). Or am I missing something?

That link states "text/javascript" is obsolete.

@natankeddem
Copy link
Contributor

natankeddem commented Sep 18, 2023

Nevermind, it might not be. This RFC confused me https://tools.ietf.org/html/rfc4329

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

I just moved the fix to nicegui.py and added a pytest.
@thetableman, @natankeddem, can you confirm that it still works on your machines?

@natankeddem
Copy link
Contributor

I was never able to "break" a fresh windows 10 install. I tried adding the key/value to 2 locations per the reports I saw. Perhaps there is more to change but unless @thetableman can't verify i'll pass on digging in further on that.

New-PSDrive -Name HKLM -PSProvider Registry -Root HKEY_LOCAL_MACHINE
New-PSDrive -Name HKCR -PSProvider Registry -Root HKEY_CLASSES_ROOT
Set-Itemproperty -path 'HKLM:\SOFTWARE\Classes\.js' -Name 'Content Type' -value 'text/plain'
Set-Itemproperty -path 'HKCR:\.js' -Name 'Content Type' -value 'text/plain'

@thetableman
Copy link
Contributor Author

I just moved the fix to nicegui.py and added a pytest. @thetableman, @natankeddem, can you confirm that it still works on your machines?

Moving mimetypes to nicegui.py also resolves the issue on my Win10 installation. I also tested with both application/javascript and text/javascript, both fixed the error I was receiving.

There's a lot of conflicting information about which to use but Mozilla, which nicegui references in other places as a primary source, reads here https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types#textjavascript that text/javascript is the current standard. This makes sense as application/... suggests a binary file type, which of course javascript is not.

@falkoschindler
Copy link
Contributor

Great, thanks @thetableman!
Then let's close this one by merging into main. 🚀

@falkoschindler falkoschindler merged commit 037c3f1 into zauberzeug:main Sep 18, 2023
1 check passed
@thetableman thetableman deleted the dynamic-properties-fix branch October 1, 2023 23:26
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.

aggrid example from the documentation does not work
4 participants