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
Introduce HTTP API for plugins #5383
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
5a2a45a
to
a32fcb8
Compare
a32fcb8
to
1d850d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design question:
Maybe rename the permission from HTTP to Network? I think this more accurately describes what the plugin will actually be allowed to do, with less of a technical description.
"Allows the plugin to make network requests over your network/over the internet/to third party websites".
Bit of an early review, haven't looked into the implementation of anything
-- Begin src/controllers/plugins/api/HTTP.hpp | ||
|
||
---@class HTTPResult | ||
---@field data string Data received from the server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data is an iffy name for this. body
or text
is a more accurate term.
Leaving these fields as methods instead means we could be more flexible when it comes to things like json parsing in the future (see https://docs.rs/reqwest/latest/reqwest/struct.Response.html#implementations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To put methods on it and have data only in C++-land I'd have to rework HTTPResult
to be a shared resource instead of just a table. I'm not sure if this is worth the complexity of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following code segfaults after a short while:
local function send_req()
local req = c2.HTTPRequest.create(c2.HTTPMethod.Get, "https://postman-echo.com/get")
req:on_success(function(res)
end)
req:on_error(function(result)
end)
req:execute()
c2.later(send_req, 100)
end
send_req()
The crash happens in the handlers of the request. Looking at the captured L
, it's probably the thread from c2.later
getting deleted while the callbacks hold on to it through lua_State
.
I'll have to store an ID of the plugin or something and get the state back then. |
Co-authored-by: nerix <nerixdev@outlook.de>
…no/chatterino2 into feature/chatterino-lua-http-api
Weird that this only happens with hosts that are remote that is have some latency. Seems like a pesky race condition. |
Let me know when it's in a finished state and I'll take a look |
This adds a new API:
c2.HTTPRequest
that requires a new permission:Network
.