-
Notifications
You must be signed in to change notification settings - Fork 177
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: add Extensions
to Request/MethodResponse
#1306
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.
Looks good to me!
Once we merge this, I think we could replace the connection_id
we expose to raw methods to Params::Extensions
. And then guarantee by our middleware that we add a unique type for connection_id.
And we'll have a way to easily pass data not only between middlewares, but also middleware -> methods
But that could happen later :D
Do we have an example of ths Extensions stuff being used? Would be great to see it in practice and offhand I can't see one :) |
tests/tests/helpers.rs
Outdated
|
||
let mut module = RpcModule::new(()); | ||
module.register_method("say_hello", |_, _, _| "hello").unwrap(); | ||
module.register_method("raw_method", |_, _, ext| *ext.get::<u32>().unwrap()).unwrap(); |
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.
Nit: I wonder whether we should rename this to eg "get_connection_id" to make it clearer what it's doing/testing?
|
||
let server_handle = server.start(module); | ||
let methods2 = methods.clone(); | ||
tokio::spawn(async move { |
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.
Ah I geuss this is all added so that we can inject connection details!
I wonder whether there's something wecan do in the future to simplify this "low level" initialising
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.
We could probably inject the connection id by default but it's quite weird to explain/document it so I didn't do it.
What does @lexnv reckon? :)
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.
I think this is fine, substrate could then inject the connection ID in a similar way
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.
This looks great! IMO it would benefit from an example showing extensions being used to eg share details between middlewares to the request method or whatever :)
Getting details in I guess requires the "low level" interface as in the test stuff, and I wonder if we can simplify injecting data on that side of things!
|
Ah awesome; I missed 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.
LGTM!
Fixes #1305
In summary this PR adds
http::Extensions
to Request and Responses this may be used to share state between middlewares and similar.Most of the changes are chore but I had to add this to all RPC handlers and the proc macro API.
Thus, the most important things to review is the changes to Request and MethodResponse.