-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
feat: added support for filters in WebServer library #9842
Conversation
👋 Hello ayushsharma82, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 56 files 56 suites 5m 11s ⏱️ Results for commit 7f11be6. ♻️ This comment has been updated with latest results. |
to implement ON_STA_FILTER and ON_AP_FILTER you are changing the API much. I suggest to split that in another PR that we could add to 3.1 or whenever makes sense to add such breaking change. |
I guess everything's backward compatible with current state of this PR? But, yeah if you want, let me know what I should split. |
Yes, please split them, because if someone has custom handlers, they will now break. We can add as breaking change when possible |
You got some errors compiling: https://github.com/espressif/arduino-esp32/actions/runs/9487829527/job/26146113347?pr=9842#step:7:61 |
Okay. Just a thought, should I try to make handlers backwards compatible 🤓? Your team can then deprecate old handlers with v3.1 or later. |
If they are backward compatible, that would be OK |
Btw, were you able to check why client IP is always 0? I looked at this log and it showed 0 too. |
localIP is fixed in #9845 |
here is one thing that I did not think about before. Because of ON_STA_FILTER & ON_AP_FILTER, WiFi will always get dragged, compiled and linked, regardless if it's used or not. How about you remove them from the classes and add them in example? Imagine a board with Ethernet. Basic sketch with ETH will be about half a megabyte, while one with WiFi will probably be more than a megabyte. |
I agree. What about creating separate files for filters which can included by user? Example: |
they are just a few lines each, so better have them inside example. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
if you have confirmed that localIP works, could you please edit the PR description? |
Tested and works well. |
Description of Change
This PR implements support for
setFilter
in WebServer library, particularly useful for conditional processing of routes based on filter functions set by user.on
methods now returnRequestHandler
which also provides another way for users to remove routes. ( Ref: Added support for removing routes in WebServer library #9832 )RequestHandler
&FunctionRequestHandler
class to addsetFilter
function.ON_STA_FILTER
&ON_AP_FILTER
Tests scenarios
I've tested this on arduino-esp32 core v3.0.1 - ESP32 ( Wemos D1 Mini ESP32 board ).
Important
Potential Bug: @me-no-dev , NetworkClient'slocalIP()
method always returnsIPAddress(0,0,0,0)
no matter through which interface the client's request came. This breaks theON_STA_FILTER
&ON_AP_FILTER
filters as we can't detect the right interface correctly.Eg. In all scenarios (WIFI_STA
/WIFI_AP
/WIFI_AP_STA
), theclient.localIP()
method return0.0.0.0
).Issue: #9843This PR is ready to be merged once the above bug is fixed.Above issue is fixed and PR is ready to be merged.
Related links
--