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

res.view() fails to render views nested more than 2 folders deep #3604

Closed
sgress454 opened this issue Feb 25, 2016 · 4 comments
Closed

res.view() fails to render views nested more than 2 folders deep #3604

sgress454 opened this issue Feb 25, 2016 · 4 comments
Assignees
Labels

Comments

@sgress454
Copy link
Member

See http://stackoverflow.com/questions/35637450/routes-in-sails-js-0-12-1

If you have a view in say views/app/user/homepage.ejs, and a route in config/routes.js like:

'/user': {view: 'app/user/homepage'}

and try to hit /user in a browser, Sails will attempt to display app/user (and fail because it doesn't exist) instead of displaying app/user/homepage.ejs.

The issue is in this code which only parses view expressions two levels deep. However, because of a previous bug in 0.11.x that was patched in 0.12, this behavior actually worked in 0.11 (the bug prevented req.options from overriding options that were initially bound with the handler, so this code had no effect and the correct view path got sent to res.view()).

We can solve this by using Lodash _.get() to parse the view expression instead of just taking the top-level and second-level view names.

@mikermcneil
Copy link
Member

Nice catch, and sounds like a plan. Only thing I'd add is that the original point of the two-level-specific view path logic was added to support the use case of using res.view() with no argument-- which is still an important use case for some apps, and something we should maintain compatibility for (including support for falling back to index). All that aside, it seems straightforward enough to fix this in https://github.com/balderdashy/sails/blob/master/lib/hooks/views/onRoute.js#L59 and still maintain compat.

@sgress454 sgress454 self-assigned this Feb 28, 2016
@sgress454
Copy link
Member Author

the original point of the two-level-specific view path logic was added to support the use case of using res.view() with no argument

This is all handled in res.view now. I think a lot of the code in the hook now is leftover from before we started using req.options for everything, and it can be way simplified. I'm writing a couple more tests and will do another PR just to get some other eyes on it since it's replacing ode code.

sgress454 added a commit that referenced this issue Feb 29, 2016
Also uses `_.get` to allow for deeply nested views in view syntax, e.g. `/foo: {view: 'foo/bar/baz'}`
refs #3604
@sgress454
Copy link
Member Author

Closed by #3617

@mikermcneil
Copy link
Member

See #3682 for follow-up on this. The issue was that (1) options needed to get passed through and (2) the target dictionary needed to get merged into the options dictionary. The first thing will always be something hook authors have to look out for-- the latter can be dealt with in the router itself (since its only relevant for core options like skipAssets and skipRegex-- other options are fine). For now, blueprints, controllers, and views hook all manage the merging of target into options themselves.

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

No branches or pull requests

2 participants