florrain / locale

Browser locale negotiation for node.js
MIT License
257 stars 36 forks source link

Npm version (0.0.12) - best not working properly #17

Closed narf closed 10 years ago

narf commented 10 years ago

The code has a problem, you return the last matching element, not the first. The version in the git repo seems to differ

Locales.prototype.best = function(locales) {
  var index, item, locale, _i, _len;

  locale = Locale["default"];
  if (!locales) {
    return this[0] || locale;
  }
  index = locales.index();
  for (_i = 0, _len = this.length; _i < _len; _i++) {
    item = this[_i];
    if (index[item]) {
      return item;
    } else {
      if (index[item.language]) {
        locale = new Locale(item.language);
      }
    }
  }
  return locale;
};

vs

best: (locales) ->
locale = Locale.default

unless locales
  return @[0] or locale

index = do locales.index

for item in @
  if index[item]
    return item
  else if index[item.language] then locale = new Locale item.language
  else
    for l in locales
      if l.language == item.language then return l

locale
jed commented 10 years ago

the code differs because this repo is coffeescript, but the version in npm is javascript. it compiles on publish,

narf commented 10 years ago

I know that.

Ok let me reformulate, the compiled version of your coffeescript differ from the compiled version of npm. The version in npm has a bug where the best function return the last locale matching the supported where it should return the first. The version in your git repo seems to be ok though.

Here is the compiled version from git repo to compare to the one i copied in my first post:

Locales.prototype.best = function(locales) {
  var index, item, l, locale, _i, _j, _len, _len1;
  locale = Locale["default"];
  if (!locales) {
    return this[0] || locale;
  }
  index = locales.index();
  for (_i = 0, _len = this.length; _i < _len; _i++) {
    item = this[_i];
    if (index[item]) {
      return item;
    } else if (index[item.language]) {
      locale = new Locale(item.language);
    } else {
      for (_j = 0, _len1 = locales.length; _j < _len1; _j++) {
        l = locales[_j];
        if (l.language === item.language) {
          return l;
        }
      }
    }
  }
  return locale;
};
jed commented 10 years ago

okay, i'm not sure how this disparity would occur, but i've recompiled and updated the npm version, so they should have parity, unless your version of coffee-script is somehow different than the current version on npm.

narf commented 10 years ago

Thank you, the code is now the same but i still have the bug.

console.log((new locale.Locales("fr-FR,en-us;q=0.5")).best(new locale.Locales(["fr", "en", "zh"])))

Write "en" in console, should return "fr" "fr-FR,en-us;q=0.5" is the string that ie8 sends me in the express request.headers["accept-language"] when i set french as first language and english as second.

narf commented 10 years ago

this line

 } else if (index[item.language]) {
  locale = new Locale(item.language);
} else {

should maybe return the locale instead of just setting the variable.

jed commented 10 years ago

accept my apologies for being a bit far from this code, @narf. i haven't used it in a long time. have you come across this bug, @stuartf?

stuartf commented 10 years ago

This is the first I've seen it, but I've verified it's happening on my master checkout.

jed commented 10 years ago

thanks, @stuartf. not sure what's going on with travis, will have to look into that.

stuartf commented 10 years ago

I opened a new issue about the travis problem.

narf commented 10 years ago

Nice, everything is working. Thanks.