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

Fixes Partial web interface #231 #247

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tobi01001
Copy link
Contributor

Script function to load css/js from the ESP when offline / error.

Script function to load css/js from the ESP when offline.
@henrygab
Copy link
Collaborator

I also put my devices onto a separate (non-internet-connected) network, so I'm interested in this change. My javascript skills are not great, so while I've looked at the PR, my review is likely insufficient.

At the same time, I do wonder ...

@jasoncoon -- Can you help me understand the benefits to obtaining the files via the internet, vs. always serving them from the device?

I ask because the reasons I immediately think of don't appear to have much impact:

  1. Bugfixes / security updates: But, specific versions are requested, so it seems the CDN versions would still give the specific versions requested?
  2. License related distribution concerns: But, each of the libraries appear to be MIT-licensed, and the code is already in the device's local file system by default?
  3. Less wireless traffic to ESP: This could be... although if the ESP provides the HTTP headers to indicate these files can be cached, web clients will only request them from the device a single time.
Apparent library list & direct links to corresponding licenses

<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-1BmE4kWBq78iYhFldvKuhfTAU6auU8tT94WrHftjDbrCEXSU1oBoqyl2QvZ6jIW3" crossorigin="anonymous">
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/jquery-minicolors/2.3.6/jquery.minicolors.min.css" integrity="sha512-BVeRnUOL0G7d4gXmj+0VxpoiQuEibKQtlkclADKvCdNrESs0LA6+H8s1lU455VqWFtHBfF/pKDGw/CMat2hqOg==" crossorigin="anonymous" referrerpolicy="no-referrer" />
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/[email protected]/font/bootstrap-icons.min.css">
<!-- Fallback to load resources from FileSystem if not available online -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question:

Would it make sense to have a common .js file, which has the common script functions?

@tobi01001
Copy link
Contributor Author

I also put my devices onto a separate (non-internet-connected) network, so I'm interested in this change. My javascript skills are not great, so while I've looked at the PR, my review is likely insufficient.

At the same time, I do wonder ...

@jasoncoon -- Can you help me understand the benefits to obtaining the files via the internet, vs. always serving them from the device?

I ask because the reasons I immediately think of don't appear to have much impact:

  1. Bugfixes / security updates: But, specific versions are requested, so it seems the CDN versions would still give the specific versions requested?
  2. License related distribution concerns: But, each of the libraries appear to be MIT-licensed, and the code is already in the device's local file system by default?
  3. Less wireless traffic to ESP: This could be... although if the ESP provides the HTTP headers to indicate these files can be cached, web clients will only request them from the device a single time.

Apparent library list & direct links to corresponding licenses

On my side, the reason is performance (so more or less 3) . The ESP does as little as neccessary (still with room for improvement though). The browser loads the stylesheets and script ressources directly from the web...
Though, when chached the impact might not be huge.

@jasoncoon
Copy link
Owner

I preferred online CDN hosted resources (css, js, etc) over local ESP hosted resources initially because the ESP webserver had problems serving multiple concurrent requests. The browser would load the html and then immediately and concurrently request all of the other resources from the ESP. Sometimes the requests would hang indefinitely, others the ESP would crash. This doesn't seem to be as much of a problem with newer library versions. With client-side caching, it's not a problem after the first reload after any changes are made.

This PR seems like a great solution, I've just needed to find time to test it. If you (@tobi01001 and/or @henrygab) have already tested it, both on and offline, I trust you and we can merge it. :)

@henrygab
Copy link
Collaborator

henrygab commented Mar 1, 2022

This PR seems like a great solution, I've just needed to find time to test it. If you (@tobi01001 and/or @henrygab) have already tested it, both on and offline, I trust you and we can merge it. :)

I have limited time these for at least the 1-2 weeks. I can only say it "looks" correct. I don't know how it acts for different browsers, although @tobi01001 shows results under chrome in both online and offline cases.

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.

3 participants