-
Notifications
You must be signed in to change notification settings - Fork 284
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
[RFC] vibe.web.web: Add more convenience methods (fullPath, session, json) #1945
base: master
Are you sure you want to change the base?
Conversation
web/vibe/web/web.d
Outdated
assert(s_requestContext.req !is null, "session() used outside of a web interface request!"); | ||
auto _session = s_requestContext.req.session; | ||
if (!_session) | ||
_session = s_requestContext.res.startSession; |
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.
That's probably the most controversial bit about this PR. My argument - this is the high-level wrapper and in the majority of the cases if there's no existent session, one wants to start one.
929c33f
to
6e1269c
Compare
I thought about it and I'd like to aim for a more functional style of the API. A few common response related functions can be added as global convenience symbols, but simple getters - especially anything query/body related - should stay accessible through parameters exclusively. It just makes the API and written code more confusing by having multiple possibilities to achieve the same thing, and such globals with trivial names are always confusing, because the declaration is hard to find in lieu of a "jump to declaration" editor feature. Basically, the order should be:
This is not saying that the approach with many convenience globals is not valid, but it doesn't fit with the initial philosophy of this module, and I'd like to avoid introducing multiple philosophies. It would also be interesting to talk about use cases. Do you have something particular in mind where these globals would currently provide a benefit and is there maybe another possible approach for getting those benefits? |
6e1269c
to
0ae60e8
Compare
Okay after getting back to this after a while, I think the only non-trivial getter is the access to the current session, so I rebased this PR to only include this one.
I was rather going for completeness. Sadly I haven't done much with Vibe.d in the last few months, but once I get back to doing more with it, I might have good use cases for it, but for now I agree that it makes sense to avoid the number of global getters - especially if they are infrequently used. |
These properties are often accessed, so imho it makes sense to add them to the list of convenience methods provided.
There's also:
fullURL
->request.fullURL
json
->request.json
path
->request.path
(I wasn't sure about this because IIRC it's planed to replace this)rootDir
->request.rootDir
(similar topath
)form
,query
->request.{form,query}
(these can be easily access via properly named parameters)files
->request.files
(rarely used)username
,password
rarely used (only once for the auth layer)cookies
- rarely used (ideally abstracted away by the session or similar)bodyReader
- can be accessed by usingInputStream
as parameter type orrequest.bodyReader
params
- can be access via_
parameterscontext
- rarely used