search

jasoncoon / esp8266-fastled-webserver

GNU General Public License v3.0
712 stars 360 forks source link

Fixes Partial web interface #231 #247

Open tobi01001 opened 2 years ago

tobi01001 commented 2 years ago

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

henrygab commented 2 years ago

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

* [jquery @ 3.6.0](https://cdnjs.cloudflare.com/ajax/libs/jquery/3.6.0/jquery.min.js) -- [MIT](https://jquery.org/license/) license * [jquery.minicolors @ 2.3.6](https://cdnjs.cloudflare.com/ajax/libs/jquery-minicolors/2.3.6/jquery.minicolors.min.css) -- [MIT](https://github.com/claviska/jquery-minicolors/blob/master/LICENSE.md) license * [bootstrap @ 5.1.3](https://cdn.jsdelivr.net/npm/bootstrap@5.1.3/dist/css/bootstrap.min.css) -- [MIT](https://getbootstrap.com/docs/5.1/about/license/) license * [bootstrap icons @ 1.7.2](https://cdn.jsdelivr.net/npm/bootstrap-icons@1.7.2/font/bootstrap-icons.min.css) -- [MIT](https://github.com/twbs/icons/blob/main/LICENSE.md) license * [FileSaver @ 2.0.5](https://cdnjs.cloudflare.com/ajax/libs/FileSaver.js/2.0.5/FileSaver.min.js) -- [MIT](https://github.com/eligrey/FileSaver.js/blob/master/LICENSE.md) license * ~~[reconnecting-websocket @ 1.0.0](https://cdnjs.cloudflare.com/ajax/libs/reconnecting-websocket/1.0.0/reconnecting-websocket.min.js)~~ -- [MIT](https://github.com/joewalnes/reconnecting-websocket/blob/master/LICENSE.txt) license, even if still commented out in current code)

tobi01001 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.