express-locale icon indicating copy to clipboard operation
express-locale copied to clipboard

Unclear interaction between `map` and `allowed`

Open arty-name opened this issue 5 years ago • 3 comments
trafficstars

The map parameter has no effect when the list of allowed locales is provided and it only includes full locale tags. This is not clear from the documentation. It this behavior desired at all?

Example configuration:

expressLocale({
  priority: ['query', 'map', 'default'],
  allowed: ['en-CA', 'fr-CA'],
  map: { en: 'en-CA', fr: 'fr-CA' },
})

With this configuration a request with query string ?locale=fr will not have locale set to fr-CA.

If this is indeed a desired behavior it would be nice to mention it in the documentation. Or is this a bug?

arty-name avatar Jan 30 '20 16:01 arty-name

Fair question. You'd indeed need to add fr to the allowed list to make this work. The whitelist should (currently) be seen as all-encompassing.

Some alternative approaches would be:

  • Apply map after every lookup (and no longer consider map as a lookup).
  • Only validate the whitelist (allowed) at the end.
  • Output these type of 'issues', to be handled (logged) further down the Express 'waterfall'.

Each has consequences that need to be investigated. Would you want to look into this further?

Sidenote: since v2, in non-production env, your config additionally throws an error because the default (en-GB) isn't whitelisted either.

smhg avatar Jan 31 '20 10:01 smhg

When I was investigating I was actually surprised to see that map is a lookup. It kind of looks like a part of configuration for allowed: here is the list of full locale tags and they have following shortcuts.

More than that, in my opinion when negotiating the locale and "fr-CA" is supported and the client asks for "fr" language it makes a lot of sense to automatically respond with "fr-CA". That might even be in the RFC for Accept-Language. Map should only be needed in case of ambiguity, like which of "fr-FR" and "fr-CA" to prefer when "fr" is requested.

I understand however that the current approach might be to provide a set of components for everyone to build the solution best suited to their unique needs, and I am wrong here to ask for an opinionated turnkey solution.

If it is the library to be modified I see yet another somewhat "automagical" approach which I use as a workaround currently: the list of allowed is extended with the keys of map. The only consequence is that when someone sends a huge map but only lists a few locales as allowed and expects all other to be rejected then it would not work as the developer expected. Is this a scenario to be supported?

There's a point in favor of not keeping map as a lookup: it has to be provided in two places. What if I list it in lookups but do not provide a map? Or other way around: what if I provide the map but do not list it in lookups? These cases may be eliminated by making map a configuration value only. And the same applies to the default actually.

arty-name avatar Jan 31 '20 14:01 arty-name

I understand however that the current approach might be to provide a set of components for everyone to build the solution best suited to their unique needs.

I think this is a good summary. There are probably as many approaches to 'detect' a user's locale as there are projects. All with subtle little differences. Some good defaults are of course preferred, but it is a delicate balance between both.

the list of allowed is extended with the keys of map

I think this is a good way forward. But even less 'magical': use the current non-production warning method to warn about un-whitelisted keys too (currently only the values are checked). That way you can never miss it.

There's a point in favor of not keeping map as a lookup: it has to be provided in two places.

Listing it in the priority list allows you to map the results of certain lookups but not of others. Not everyone adds a whitelist. I don't think that approach is too bad.

smhg avatar Jan 31 '20 14:01 smhg