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

[RFC] vibe.web.web: Add more convenience methods (fullPath, session, json) #1945

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Oct 10, 2017

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 to path)
  • 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 using InputStream as parameter type or request.bodyReader
  • params - can be access via _ parameters
  • context - rarely used

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;
Copy link
Member Author

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.

@s-ludwig
Copy link
Member

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:

  1. Rarely used features shouldn't be wrapped to keep the API compact, but the user can simply access them through a HTTPServerRequest/HTTPServerResponse parameter
  2. Frequently used features should be made accessible through parameters if possible
  3. Only frequently used features that don't fit the parameter approch should end up as global functions

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?

@wilzbach
Copy link
Member Author

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.

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.
If session is to general, one could rename it to currentSession.

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?

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.

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.

None yet

2 participants