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

sails.hooks.views.render() : Render view, in non request scope, do not take locale into account #3833

Closed
jberrisch opened this issue Sep 8, 2016 · 8 comments

Comments

@jberrisch
Copy link

Sails version:0.12.4
Node version:4.5.0
NPM version:2.15.9
Operating system:Os X 10.11.6


I try to use a hook (sails-hook-cron) to send emails to customers who have different locales.
In this hook I use 'sails.hooks.views.render' to compile my email template.
This works quite fine but when I pass the locale as options it is ignored.
Here is a example hook to reproduce this error:

module.exports = function(sails) {
    return {
        initialize: function(cb) {
            sails.on('lifted', function() {
                if (sails.hooks.views && sails.hooks.views.render) {
                    sails.hooks.views.render('email/test', {
                        locale: 'fr'
                    }, (err, compiledEmail) => {
                        console.log(compiledEmail);
                    });
                }
            });

            return cb();
        }
    }
};

I got into the source code and at the ligne 102 of the file: node_modules/sails/lib/hooks/views/render.js the 'req.locale' is set to 'options.locale'.

var req = {
  headers: {}
};

// Initialize i18n if hook is enabled
if (sails.hooks.i18n) {
  require('i18n').init(req, options, function() {

    // Set the locale if necessary
    if (options.locale) {
      req.locale = options.locale;
    }

    // Render the view
    sails.config.views.engine.fn(absPathToView, options, cb_view);
  });
}

But, ... the i18n module invoked at ligne 98 (require('i18n').init) overrides the 'options.locale' value.
So at ligne 102 it will always be set to the default locale because i18n guesses the value from the url-params / headers / cookies but never from the options.

By adding:

req.headers['accept-language'] = (options.locale || sails.hooks.i18n.defaultLocale);

after ligne 97 (render.js) it works like expected, but I guess this is not very beautiful.

I tried to write a pull request and a test for this issue, but I had some trouble charging the i18n hook for the test because there's no sample configuration...

@Josebaseba
Copy link
Contributor

I can confirm that with sails v0.11.5 it's working properly.

@jberrisch
Copy link
Author

I downgraded my sails (0.12.4) i18n module to version 0.5.0 (dependency of sails 0.11.5) and it works fine.
They (i18n team) changed a lot in how the .init function works...

@Josebaseba
Copy link
Contributor

This feature it's really important for my app, so when I find some time I'll try to find a solution and then send a PR. I want to update to sails v0.12 but I need this functionality running!

@mikermcneil
Copy link
Member

@Josebaseba That'd be awesome! Talked to @dougwilson a little bit about this recently, but unfortunately it sounds like there's still a ways to go before views support will be ready to extrapolate from Express.

In the mean time, if you're able to get this working, then we can make sails.renderView() (aka sails.hooks.views.render) officially supported & documented for Sails v1

@mikermcneil mikermcneil changed the title Render view, in non request scope, do not take locale into account sails.hooks.views.render() : Render view, in non request scope, do not take locale into account Oct 20, 2016
@Josebaseba
Copy link
Contributor

Done 👍 #3889

@jberrisch
Copy link
Author

Wonderful news, thanks a lot!

mikermcneil added a commit that referenced this issue Nov 16, 2016
[fixes #3833] Take locale into account in views.render()
@Josebaseba
Copy link
Contributor

Josebaseba commented Nov 16, 2016

@jberrisch please can you test it? npm install sails@^0.12.10-1

For me it's working as it should, take a look and then we can close this issue. Thanks!

@mikermcneil
Copy link
Member

@Josebaseba @jberrisch thanks guys!

(Now published as 0.12.10)

@mikermcneil mikermcneil added waiting to close This label is deprecated. Please don't use it anymore. and removed enhancement labels Nov 17, 2016
@sailsbot sailsbot removed the waiting to close This label is deprecated. Please don't use it anymore. label Nov 21, 2016
sgress454 pushed a commit that referenced this issue Nov 22, 2016
[fixes #3833] Take locale into account in views.render()
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

4 participants