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

Expose target action in req object #1372

Closed
marionebl opened this issue Feb 4, 2014 · 24 comments
Closed

Expose target action in req object #1372

marionebl opened this issue Feb 4, 2014 · 24 comments

Comments

@marionebl
Copy link
Contributor

The req.target object was removed on the road to v0.10 (discussed here, commit 409f744) and replaced by req.options.

This is all nice looking at it from the hooks and configuration side, but the options object does not contain information about the target action (formerly req.target.action) the request will land on, e.g.

{ detectedVerb: { verb: '', original: '/user/:id?', path: '/user/:id?' },
  actions: false,
  rest: true,
  shortcuts: false,
  prefix: '',
  pluralize: false,
  index: true,
  model: 'user' }

exposes not enough information to get the target action in a sane way. That makes it really hard to write solid policies or views that need to know about the target action without mimicking a big part of the sails router logic (read: I was not able to find one).

Now to what this boils down is the following proposal: Expose the target action in the request object again, e.g. like this:

{ detectedVerb: { verb: '', original: '/user/:id?', path: '/user/:id?' },
  target: { action: 'find', controller: 'user' },
  actions: false,
  rest: true,
  shortcuts: false,
  prefix: '',
  pluralize: false,
  index: true,
  model: 'user' }

Related SO question: stackoverflow.com/../get-sails-request-target-from-policy-scope
Related Google Groups posting: https://groups.google.com/forum/#!topic/sailsjs/faX54hqcNZE

@RWOverdijk
Copy link
Contributor

I think you should close this issue. It's not an issue, but a proposal.

@Vadorequest
Copy link

Don't close it and add a label then, would be better!
Like "Discussion" or "Proposal".
We can talk about it on Stackoverflow or here, duplicate threads on several website is kinda difficult to follow.

@RWOverdijk
Copy link
Contributor

@Vadorequest Issues are for issues. Discussions are for google groups. Stack overflow is more of an "FAQ" thing.

@Vadorequest
Copy link

Google group is a fucking bullshit thing. Absolutely useless, just the fact
to have to wait like sometimes 3 days before being accepted is just ...
Awesome.
Issue can be tagged and ... Why should we talk about sails in
stackoverflow? I mean that should stay here, it's maybe not the way it
works but I don't like it.
Github should be used to deal with both. Not use another application to
talk about "enhancement" or whatever!

2014-02-07 14:20 GMT+01:00 Wesley Overdijk notifications@github.com:

@Vadorequest https://github.com/Vadorequest Issues are for issues.
Discussions are for google groups. Stack overflow is more of an "FAQ" thing.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1372#issuecomment-34435622
.

Cordialement,

M. Ambroise Dhenain.

@RWOverdijk
Copy link
Contributor

I'm opting out of this conversation. But watch your language.

@mikedevita
Copy link
Contributor

according to the contributors requests via CONTRIBUTING.md new features should be requested on Trello.. not github.

https://github.com/balderdashy/sails/blob/master/CONTRIBUTING.md

@Vadorequest
Copy link

Good to know, thanks.

2014-02-08 16:54 GMT+01:00 Mike DeVita notifications@github.com:

according to the contributors requests via CONTRIBUTING.md new features
should be requested on Trello.. not github.

https://github.com/balderdashy/sails/blob/master/CONTRIBUTING.md


Reply to this email directly or view it on GitHubhttps://github.com//issues/1372#issuecomment-34547320
.

Cordialement,

M. Ambroise Dhenain.

@mikermcneil
Copy link
Member

Thanks @mikedevita, @RWOverdijk, and @Vadorequest!

@marionebl req.options.action === req.target.action. The idea of bringing everything into req.options was to reduce our footprint on req, but I agree, it's a lot to understand without any proper docs on the subject :\ As a first step to that end, here's a bit more on req.options :

  • req.options is an object which contains information about the configuration for the current route. It can be extended manually by passing in extra properties in your explicit routefile, i.e. config/routes.js. So 'GET /foo/bar': { controller: 'foo', action: 'bar', baz: 'wiccanBroomTournament' } would give us req.options.baz, req.options.controller, and req.options.action.
  • req.options is also automatically populated using information from blueprint configuration, and provides a living snapshot of the understanding Sails has about a particular request at any given time.
  • req.options is used by Sails built-in blueprint actions to allow them to infer things like the model, JSONP callback parameter, etc. We need some way to do this so that the blueprint actions can stay utterly generic (and therefore reusable across different use cases.)
  • Based on that, there is an interesting use case for req.options in the future-- we can use it to configure blueprint actions rather than writing our own custom controller actions from scratch. So.. yeah-- it could allow us (in conjunction with generators) to create a plugin ecosystem for easily sharing generic logic across each other's apps. This is very exciting to me, because I get sick of writing the same damn thing over and over :) Before we proceed too far down that road, however, I'd like to give the community (and myself) time to experiment with using req.options in our own custom blueprints/controller actions in order to discover some best practices, since that part is very much an experimental concept.

Re: your suggestion-- I'm on board with splitting req.options up again in a future version, but I'd like to have a very clear idea on the advantages and the details of the implementation before we jump into it (to protect us all from inadvertently adding unneeded complexity or bugs).

Thanks for bringing this up!

@Vadorequest
Copy link

Thanks Mike!

I'm still using the 0.9.8 version and I won't use the 0.10 until it's
featured. But what I know for sure is that I will always need to know the
controller and the action, because I have my custom controller management.
(I will create a git in a few days with custom controller/model and
inheritance based on sails and Typescript, could bring some ideas to the
community!)

So, whatever there is a bueprint or not, I think that it would be better to
always know the controller/action, doesn't matter if it's in req.target or
req.options, this part is just a little change in the source code!

2014-02-10 2:38 GMT+01:00 Mike McNeil notifications@github.com:

Thanks @mikedevita https://github.com/mikedevita, @RWOverdijkhttps://github.com/RWOverdijk,
and @Vadorequest https://github.com/Vadorequest!

@marionebl https://github.com/marionebl req.options.action ===
req.target.action. The idea of bringing everything into req.options was
to reduce our footprint on req, but I agree, it's a lot to understand
without any proper docs on the subject :\ As a first step to that end,
here's a bit more on req.options :

  • req.options is an object which contains information about the
    configuration for the current route. It can be extended manually by passing
    in extra properties in your explicit routefile, i.e. config/routes.js.
    So 'GET /foo/bar': { controller: 'foo', action: 'bar', baz:
    'wiccanBroomTournament' } would give us req.options.baz,
    req.options.controller, and req.options.action.
  • req.options is also automatically populated using information from
    blueprint configuration, and provides a living snapshot of the
    understanding Sails has about a particular request at any given time.
  • req.options is used by Sails built-in blueprint actions to allow
    them to infer things like the model, JSONP callback parameter, etc. We need
    some way to do this so that the blueprint actions can stay utterly generic
    (and therefore reusable across different use cases.)
  • Based on that, there is an interesting use case for req.options in
    the future-- we can use it to configure blueprint actions rather than
    writing our own custom controller actions from scratch. So.. yeah-- it
    could allow us (in conjunction with generators) to create a plugin
    ecosystem for easily sharing generic logic across each other's apps. This
    is very exciting to me, because I get sick of writing the same damn thing
    over and over :) Before we proceed too far down that road, however, I'd
    like to give the community (and myself) time to experiment with using
    req.options in our own custom blueprints/controller actions in order
    to discover some best practices, since that part is very much an
    experimental concept.

Re: your suggestion-- I'm on board with splitting req.options up again in
a future version, but I'd like to have a very clear idea on the advantages
and the details of the implementation before we jump into it (to protect us
all from inadvertently adding unneeded complexity or bugs).

Thanks for bringing this up!


Reply to this email directly or view it on GitHubhttps://github.com//issues/1372#issuecomment-34594948
.

Cordialement,

M. Ambroise Dhenain.

@marionebl
Copy link
Contributor Author

Thank you for the insight on the reasoning behind req.options @mikermcneil. I like the snapshot concept and see the potential for nice'n'generic blueprints, too!

To clear things up on this issue: Just like @Vadorequest said the actual problem for me was that requests hitting blueprints don't receive an action key in req.options as opposed to requests hitting custom controller actions (which I did not realize when writing the issue - thanks for pointing this out; @RWOverdijk via IRC, too).

That's perfectly logical taking the way req.options is constructed into account and consistent with the snapshot concept. I'll come up with an own lookup for blueprint requests then.

Closing this issue.

@artworkad
Copy link
Contributor

I use sails 0.10 rc2 and

req.options

is missing both action and controller information no matter what blueprint route I hit. The only usefull value is the model. It was easier to write policies in sails 0.9x.

@marionebl
Copy link
Contributor Author

@artworkad I came up with a simple utility method that derives the target action from the request object. Have a look at my answer on SO: http://stackoverflow.com/a/21683181/3263412. Getting the controller can safely be done via: var controller = req.options.controller || req.options.model;

@artworkad
Copy link
Contributor

@marionebl thanks, but sails complains about

require('lodash')

It cannot find the module. Any ideas on this?

@marionebl
Copy link
Contributor Author

You'll have to install lodash for this to work: npm install --save lodash.

@artworkad
Copy link
Contributor

@marionebl Got it, I can get controller but your service returns undefined for the action :/

@artworkad
Copy link
Contributor

So this works for me now. E.g. there is an action book on the ticket model:

if req.options.detectedVerb.original.indexOf('book') != -1
    then "you just hit the book action"

but not absolutely secure ... :/

@marionebl
Copy link
Contributor Author

Hey, I guess you could get detailed help (from more devs!) via IRC (irc.freenode.org, channel #sailsjs).

Contribution Guide | Stackoverflow | Google Group | Trello | IRC

@mikermcneil
Copy link
Member

@Vadorequest @marionebl thanks for taking the time to explain, guys. I'm on the same page now. And you're right.

@mikermcneil
Copy link
Member

@marionebl the service you put together on Stackoverflow helps a lot

I'll play with it for a bit and see if we can make it simpler to grab hold of the relevant information. If anyone has any ideas in the meantime, let me know!

@Vadorequest
Copy link

@mikermcneil I updated my sails app (https://github.com/balderdashy/sails-docs/issues/242#issuecomment-52510987) and couldn't find how to fix this, I just saw that it's available from req.options.action instead of req.target.action!

Maybe this change should also be documented in the doc that explains what changed from 0.9.x to 0.10.x!

@lucasmonstrox
Copy link

Some news about how to get action and controller? :'(

@RWOverdijk
Copy link
Contributor

@luqezman req.options.controller and req.options.action

@lucasmonstrox
Copy link

lucasmonstrox commented Sep 1, 2016

@RWOverdijk this options is only available after "route" middleware runs :'( Inside a hook using "after route" listener, is impossible to get it 👎

@RWOverdijk
Copy link
Contributor

I'm not familiar with the after route listener, so I can't help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants