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

Rewrite Basic-Web_Skeleton_App to reflect comments on github/gitter #669

Closed

Conversation

avr39-ripe
Copy link
Contributor

#668 - can be merged separately, but merge of this PR is PREFERABLE prior to merge this PR
#666 - is MANDATORY for this PR.

Also for proper function of WebSkeletonApp in different networking env PR #631 + #622 SHOULD be merged

Fully rewrite WebSkeletonApp, most notable changes:

  • Whole app is now split into two classes ApplicationClass and ApplicationConfig
  • ApplicationConfig instanced in ApplicationClass as public Config - it should be used as very global source of configuration for application
  • there is instance of HttpServer in ApplicationClass named webServer - it should be used for add own path handlers for application
  • WifiStation and WifiAccesPoint are configured once if there is empty sdk configuration area on flash
  • Wifi Station mode SSID and PASSWORD are stored in sdk configuration area on flash, no need to double it in own json config
  • Rewrite javascript without jqery
  • some other bugs fixed (and for sure - new introduced)

@MrAPierce
Copy link
Contributor

I wouldn't use the fetch javascript method as a replacement for jquery ajax/get/post or the standard XMLHttpRequest, as fetch is not widely supported http://caniuse.com/#feat=fetch. It would be underwhelming to load this to a esp with this example and have it fail because of a javascript error, as it would most likely be the place to start for a new comer to sming.

I'm guessing that the reason for moving away from jquery is to save space and limit additional requests, but if you go down the path of using fetch you will have to use a polyfill such as https://github.com/github/fetch to overcome the browser support issues, which in turn adds another request to the esp anyway.

Just my opinion but I would use widely supported javascript api calls or jquery whilst writing any web related examples for sming.

Cheers.

@avr39-ripe
Copy link
Contributor Author

@MrAPierce Good comment! thanks! Removal of jquery was for several reasons: as you guess - size reduction, useless of jquery for this app as all jquery do can be done with modern js itself if we do not need ANCIENT browsers, so I think pure js is better then teach newcomers rather bad practice to use jquery everywhere even for trivial getelementById... about fetch - it is future, not so far future and it syntax is much more clear and simple even for people who do not fully understand promises and what really happen under the hood :) With fetch it will be easier for newcomers to write their get/post requests. people tend to use what they see for the first time :) But as you said fetch isn't natively supported by all major browsers, so I think using polyfill wil be good compromise.. Will gladly hear any further comments!

@patrickjahns
Copy link
Member

@avr39-ripe
I can see your point - but I agree with @MrAPierce. Even though I don`t own a mac/use it - I know many people that use Macs and unfortunately they also rely on Safari.

I second to include the polyfill in order to keep cross compatibility

@avr39-ripe
Copy link
Contributor Author

@MrAPierce @patrickjahns So I add fetch polyfill to web ui

@hreintke
Copy link
Contributor

hreintke commented Apr 3, 2016

@avr39-ripe :
I second the remarks from @patrickjahns on example vs framework.
The positive is that it is moving away from the "C-type" example which are the majority within Sming.
The application class however has the same linear structure. The differentiation between pubic and private variables/functions is unclear.
There is no error-checking on results f.e :

 +      JsonObject& root = jsonBuffer.parseObject(jsonString);
+
+       loopInterval = root["loopInterval"];

On the "loop" :
I disagree with including a "loop" within the basic example. Sming is in essence callback/event based and that should be the focus.

In short : it is a good start for a better example but definitely needs some more work.
Next comments will follow.

@slaff
Copy link
Contributor

slaff commented May 17, 2017

@avr39-ripe If there are useful changes in this PR can you rebase it according to the latest develop?

@avr39-ripe
Copy link
Contributor Author

it is outdated mostly. Bit later will publish my recent work for things like this WebSkeletonApp

@slaff slaff closed this May 17, 2017
@slaff slaff removed the Review label May 17, 2017
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.

5 participants