Skip to content

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

Open
wilzbach wants to merge 1 commit intovibe-d:masterfrom
wilzbach:convenience-web-methods
Open

[RFC] vibe.web.web: Add more convenience methods (fullPath, session, json)#1945
wilzbach wants to merge 1 commit intovibe-d:masterfrom
wilzbach:convenience-web-methods

Conversation

@wilzbach
Copy link
Copy Markdown
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

Comment thread web/vibe/web/web.d
{
assert(s_requestContext.req !is null, "session() used outside of a web interface request!");
auto _session = s_requestContext.req.session;
if (!_session)
Copy link
Copy Markdown
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
Copy Markdown
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 wilzbach force-pushed the convenience-web-methods branch from 6e1269c to 0ae60e8 Compare November 30, 2017 17:26
@wilzbach
Copy link
Copy Markdown
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.

2 participants