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

Unclear interaction between map and allowed #20

Open
arty-name opened this issue Jan 30, 2020 · 3 comments
Open

Unclear interaction between map and allowed #20

arty-name opened this issue Jan 30, 2020 · 3 comments

Comments

@arty-name
Copy link

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?

@smhg
Copy link
Owner

smhg commented Jan 31, 2020

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 added a commit that referenced this issue Jan 31, 2020
@arty-name
Copy link
Author

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.

@smhg
Copy link
Owner

smhg commented Jan 31, 2020

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.

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

No branches or pull requests

2 participants