doshprompt / angular-localization

angularjs localization done right.
http://doshprompt.github.io/angular-localization
MIT License
159 stars 78 forks source link

i18n filter doesn't work in v1.1.2 #15

Closed okayestdeveloper closed 9 years ago

okayestdeveloper commented 10 years ago

I have a fairly large app in development that uses the i18n filter in angular-localization quite a bit. I was using 1.0.1 and just upgraded to 1.1.2 and the filter stopped working. The attribute directive and service still work as expected. I confirmed this behavior in the demo app included in this repository.

I stepped into the code, and it looks like the filter fires when expected and causes the language files to load from the server, but the strings are never replaced after the language file is loaded.

I also upgraded Angular to 1.3.0 at the same time, though I don't know if that matters.

doshprompt commented 10 years ago

That's odd, it's untested with 1.3, I should probably add that as a dependency to bower. However, are you using ngSanitize? I believe that it's now a dependency so you will need to include the angular-sanitize.min.js script in your app. Please let me know if this solves your problem or else I will dig deeper

okayestdeveloper commented 10 years ago

I did include angular-sanitize.min.js in the app. Even if I hadn't, that wouldn't account for the filter not working in the demo app.

doshprompt commented 10 years ago

@bradledbetter that seems odd, the unit tests run fine but I did notice in an actual app in 1.3 the filter is indeed broken, but the filter is firing like you said and causing the file load to trigger. I have a feeling it's to do with promises inside filters, I will take a look and hopefully have a fix soon. If you want you can also open a PR with it but I am already on it. Hope this helps.

okayestdeveloper commented 10 years ago

That is odd. I'll probably just roll back my angular upgrades in the morning when I get back at it and then just upgrade angular-localization to see how that works.

okayestdeveloper commented 10 years ago

Whoops. Darn shaky mouse hand clicked the wrong button.

doshprompt commented 10 years ago

@bradledbetter I've confirmed that i18n and i18n attr directive remain unaffected, maybe switch to using the directive and keep ng 1.3? (though I know it's a pain to migrate all of the filters to directives, I've had to do it once before on a large scale app, but we did notice significant perf improvements when the overhead dropped so maybe that's the silver lining?)

PS: I am still investigating a potential fix for this, so far I've come up short though :( Feel free to jump in and help.

okayestdeveloper commented 10 years ago

This morning I locked my app to angular 1.2.26, rolled back angular-localization to 1.0.1, updated and rebuilt. Filter and directive worked as expected. I then started walking angular-localization forward one release at a time to see if I could find where the filter broke down. I've gotten it to 1.1.3 now and the filter, directive, and service all replace strings as expected. So it must be some incompatibility with angular 1.3.0. I quickly read through the changelog ( https://github.com/angular/angular.js/blob/master/CHANGELOG.md ) but nothing jumped out at me. I'll step through the code to see if I can find anything.

doshprompt commented 10 years ago

@bradledbetter Yeah I've been doing the same thing, seems like only the filter is incompatible with 1.3 though, I think it has to do with async timing and the way they now handle digest cycles, the filter may not refire once the file has been loaded. We'll get to the bottom of this soon.

okayestdeveloper commented 10 years ago

@doshprompt I think you're right about the root cause. I finally got around to forking my project and comparing execution between 1.2.6 and 1.3.0. I'm not familiar with the details of the inner workings of Angular, so it's been an education. I think perhaps if locale.getLocalizedString returned a promise that gets resolved with the localized string when the bundle is loaded, that might work better with the new digest cycle.

I don't know about the best way to go about working that in without breaking other things in angular-localization, but I'll give it a shot when I get a chance.

doshprompt commented 10 years ago

@bradledbetter I'll try your suggested approach and see what happens, I think angular deprecated automatic promise unwrapping though.

okayestdeveloper commented 10 years ago

@doshprompt Yeah...I've been looking at it between different projects today. I'm trying to make as small an impact as possible, and I think maybe changing locale.getBundle to including loading the bundle and use the new $q constructor and then resolving/rejecting the promise when http.get returns. Something along these lines:

function getBundle(txt) {
  return  $q(function(resolve, reject) {
    if ( bundle is loaded ) {
      resolve(bundle);
    } else {
      $http.get({}).
        success(function() {
          resolve(bundle);
        }).
        fail(function() {
          reject(error);
        });
    }
  });
}

And then in locale.getLocalizedString:

                  var promisedBundle = getBundle(txt);
                  promisedBundle.then(function(bundle){
                    key = getKey(txt);

                    if (bundle[key]) {
                      result = applySubstitutions(bundle[key], subs);
                    } else {
                      $log.info("[localizationService] Key not found: " + txt);
                      result = "%%KEY_NOT_FOUND%%";
                    }
                  });

to replace the section after bundle = getBundle(txt). Now that I look at it, I think you'd have to wrap all of getLocalizedString in the promise.then() to get it to work correctly.

The $q constructor form is new in 1.3.0, so that breaks backwards compatibility with 1.2.x, but the code could be rewritten.

doshprompt commented 10 years ago

@bradledbetter you're right getLocalizedString will need to be wrapped in a promise.then() in the directives and wherever the service is being used directly too

Let me look into $q.constructor, I didn't know about that as I haven't dabbled with 1.3 much. Meanwhile, if you want to make your changes in place in angular-localization.js go ahead and test them out, then we can work on maintaining backwards compatibility.

Kageetai commented 9 years ago

I have the same issue and wanted to use the filter because it looks cleaner in the code. Now you say the directive is faster so it might be better using this, than downgrading to Angular 1.2 just for using the filter?

doshprompt commented 9 years ago

Hi all,

Apologies for the delay. I am working on a more scaleable and intelligent behind-the-scenes fix for this, but for now one possible workaround if you must use filters directly in your views is the following:

In your router (ngRoute or ui-router) you can add the locale.ready to your list of resolves. But the one caveat is that the list must be maintained by hand. So if you introduce a new localized resource file and address at least one of its keys in the view, that file must be added to the list in the resolves like this:

$routeProvider.when('/example', {
  templateUrl: 'views/example.tmpl.html',
  controller: 'ExampleCtrl',
  resolve: {
    langs: function (locale) {
      return locale.ready('common');
    }
  }
});

Fortunately, locale.ready supports accepting multiple file paths so you don't need a separate resolve for each lang file but instead can add all of them by modifying the above example like so:

$routeProvider.when('/example', {
  templateUrl: 'views/example.tmpl.html',
  controller: 'ExampleCtrl',
  resolve: {
    langs: function (locale) {
      return locale.ready([
        'common',
        'example'
      ]);
    }
  }
});

Please let me know if this helps and works well for you, in the meantime I will continue to work on a better solution.

jbertoglio commented 9 years ago

Fixed it myself.

gulp.task('server', ['less', 'jshint:app', 'connect:lr', 'watch']); gulp.task('default', ['less', 'jshint:app', 'connect:lr', 'watch']);

Adding the second line appears to work.

jb

On Mon, Dec 1, 2014 at 2:39 PM, John Bertoglio jbertoglio@gmail.com wrote:

On a related note: the ng-scaffolding project looks interesting but when you run gulp it complains that there is no "default" task. Am I missing something?

jb

On Mon, Dec 1, 2014 at 2:21 PM, Rahul Doshi notifications@github.com wrote:

Hi all,

Apologies for the delay. I am working on a more scaleable and intelligent behind-the-scenes fix for this, but for now one possible workaround if you must use filters directly in your views is the following:

In your router (ngRoute or ui-router) you can add the locale.ready to your list of resolves. But the one caveat is that the list must be maintained by hand. So if you introduce a new localized resource file and address at least one of its keys in the view, that file must be added to the list in the resolves like this:

$routeProvider.when('/example', { templateUrl: 'views/example.tmpl.html', controller: 'ExampleCtrl', resolve: { langs: function (locale) { return locale.ready('common'); } } });

Fortunately, locale.ready supports accepting multiple file paths so you don't need a separate resolve for each lang file but instead can add all of them by modifying the above example like so:

$routeProvider.when('/example', { templateUrl: 'views/example.tmpl.html', controller: 'ExampleCtrl', resolve: { langs: function (locale) { return locale.ready([ 'common', 'example' ]); } } });

Please let me know if this helps and works well for you, in the meantime I will continue to work on a better solution.

— Reply to this email directly or view it on GitHub https://github.com/doshprompt/angular-localization/issues/15#issuecomment-65145468 .

John A. Bertoglio Co-Laboratory, Inc. 503.330.6713

John A. Bertoglio Co-Laboratory, Inc. 503.330.6713

raphox commented 9 years ago

Thanks @doshprompt

Im using 'ui.router' and now it is working. My code is:

(function() {
  angular.module("ppaUi", ['ionic', 'ngLocalize', 'restangular', 'ui.router']).config([
    '$stateProvider', '$urlRouterProvider', function($stateProvider, $urlRouterProvider) {
      $stateProvider.state("home", {
        url: "/",
        templateUrl: "app/main/main.tmpl.html",
        controller: "MainCtrl"
      }).state("login", {
        url: "/login",
        templateUrl: "app/auth/auth.tmpl.html",
        controller: "LoginCtrl",
        resolve: {
          langs: function(locale) {
            return locale.ready('auth');
          }
        }
      });
      return $urlRouterProvider.otherwise('/login');
    }
  ]);

}).call(this
raphox commented 9 years ago

@doshprompt When I use 'locale.setLocale(loc);' occurs same problem when I dont have the resolve on stateProvider. When I call 'locale.setLocale(loc);' only directives receive translations, in filter i18n nothing happens.

abdheshkumar commented 9 years ago

@doshprompt How can i change locale at run time by clicking on button?? Here is my code example but it's not changing locale.

app.controller('exampleCtrl', [ '$scope', 'locale', 'localeEvents', function($scope, locale, localeEvents) { $scope.changeLocale = function() { console.log("::::I:::::" + locale.getLocale()) locale.setLocale("fr-FR"); console.log("::::II:::::" + locale.getLocale()) } } ]);

doshprompt commented 9 years ago

@abdheshkumar what version of angular are you running? Also, can you create a plunker/jsfiddle to reproduce the problem, it will be easier for me to debug there.

ecancil commented 9 years ago

Has this been fixed yet? When i use your router fix i get the following error

Error: [$rootScope:inprog]