Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/default-search.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,6 @@ module.exports = {
],
},
},

redirectLangPrefix: false,
};
2 changes: 2 additions & 0 deletions config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ module.exports = {
},
rtlLangs: ['ar', 'dbr', 'fa', 'he'],
defaultLang: 'en-US',
redirectLangPrefix: true,

localeDir: path.resolve(path.join(__dirname, '../locale')),

// This is off by default
Expand Down
20 changes: 18 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"eslint": "eslint .",
"stylelint": "stylelint --syntax scss **/*.scss",
"lint": "npm run eslint && npm run stylelint",
"servertest": "ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest",
"servertest": "ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:disco && better-npm-run servertest:search",
"start": "npm run version-check && NODE_PATH='./:./src' node bin/server.js",
"test": "better-npm-run test",
"unittest": "better-npm-run unittest",
Expand Down Expand Up @@ -63,12 +63,28 @@
}
},
"servertest": {
"command": "mocha --compilers js:babel-register --timeout 10000 --recursive tests/server/",
"command": "mocha --compilers js:babel-register --timeout 10000 tests/server/",
"env": {
"NODE_PATH": "./:./src",
"NODE_ENV": "production"
}
},
"servertest:disco": {
"command": "mocha --compilers js:babel-register --timeout 10000 tests/server/disco",
"env": {
"NODE_PATH": "./:./src",
"NODE_ENV": "production",
"NODE_APP_INSTANCE": "disco"
}
},
"servertest:search": {
"command": "mocha --compilers js:babel-register --timeout 10000 tests/server/search",
"env": {
"NODE_PATH": "./:./src",
"NODE_ENV": "production",
"NODE_APP_INSTANCE": "search"
}
},
"test": {
"command": "npm run version-check && npm run unittest && npm run servertest && npm run eslint && npm run stylelint",
"env": {
Expand Down
3 changes: 1 addition & 2 deletions src/amo/routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import App from './containers/App';
import Home from './containers/Home';

export default (
<Route path="/" component={App}>
<Route path="/(:lang/)" component={App}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be path="/(:lang/)firefox/" but that could wait if you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how this will work if you get something like /firefox/addons/, would it be smart enough to see that there is no lang? Maybe we shouldn't worry about redirecting the lang other than at /?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<IndexRoute component={Home} />
<Route path="/:lang/" component={Home} />
</Route>
);
4 changes: 2 additions & 2 deletions src/core/client/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { render } from 'react-dom';
import { Provider } from 'react-redux';
import { Router, browserHistory } from 'react-router';
import { ReduxAsyncConnect } from 'redux-async-connect';
import { langToLocale, getLanguage } from 'core/i18n/utils';
import { langToLocale, sanitizeLanguage } from 'core/i18n/utils';
import I18nProvider from 'core/i18n/Provider';
import Jed from 'jed';

Expand All @@ -18,7 +18,7 @@ export default function makeClient(routes, createStore) {
let initialState;

const html = document.querySelector('html');
const lang = getLanguage(html.getAttribute('lang'));
const lang = sanitizeLanguage(html.getAttribute('lang'));
const locale = langToLocale(lang);
const appName = config.get('appName');

Expand Down
95 changes: 81 additions & 14 deletions src/core/i18n/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ export function normalizeLocale(locale) {
return langToLocale(localeToLang(locale));
}

export function isValidLang(lang) {
return validLangs.includes(lang);
export function isValidLang(lang, { _validLangs = validLangs } = {}) {
return _validLangs.includes(normalizeLang(lang));
}

export function getLanguage(langOrLocale) {
export function sanitizeLanguage(langOrLocale) {
let language = normalizeLang(langOrLocale);
// Only look in the un-mapped lang list.
if (!langs.includes(language)) {
Expand All @@ -73,24 +73,91 @@ export function getLanguage(langOrLocale) {
}

export function isRtlLang(lang) {
const language = getLanguage(lang);
const language = sanitizeLanguage(lang);
return rtlLangs.includes(language);
}

export function getDirection(lang) {
return isRtlLang(lang) ? 'rtl' : 'ltr';
}

export function getLangFromRouter(renderProps) {
if (renderProps) {
// Get the lang from the url param by default
// if it exists.
if (renderProps.params && renderProps.params.lang) {
return getLanguage(renderProps.params.lang);
} else if (renderProps.location && renderProps.location.query &&
renderProps.location.query.lang) {
return getLanguage(renderProps.location.query.lang);

function qualityCmp(a, b) {
if (a.quality === b.quality) {
return 0;
} else if (a.quality < b.quality) {
return 1;
}
return -1;
}

/*
* Parses the HTTP accept-language header and returns a
* sorted array of objects. Example object:
* { lang: 'pl', quality: 0.7 }
*/
export function parseAcceptLanguage(header) {
// pl,fr-FR;q=0.3,en-US;q=0.1
if (!header || !header.split) {
return [];
}
const rawLangs = header.split(',');
const langList = rawLangs.map((rawLang) => {
const parts = rawLang.split(';');
let q = 1;
if (parts.length > 1 && parts[1].trim().indexOf('q=') === 0) {
const qVal = parseFloat(parts[1].split('=')[1]);
if (isNaN(qVal) === false) {
q = qVal;
}
}
return {
lang: parts[0].trim(),
quality: q,
};
});
langList.sort(qualityCmp);
return langList;
}


/*
* Given an accept-language header and a list of currently
* supported languages, returns the best match with no normalization.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This says "with no normalization" but isValidLang is _validLangs.includes(normalizeLang(lang)) which appears to normalize?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess this would match en-CA to en and then return en-CA? Is that wanted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind. It would treat en-ca as en-CA and not match, then pick en since it matches without a region.

*
*/
export function getLangFromHeader(acceptLanguage, { _validLangs } = {}) {
let userLang;
if (acceptLanguage) {
const langList = parseAcceptLanguage(acceptLanguage);
for (const langPref of langList) {
if (isValidLang(langPref.lang, { _validLangs })) {
userLang = langPref.lang;
break;
// Match locale, even if region isn't supported
} else if (isValidLang(langPref.lang.split('-')[0], { _validLangs })) {
userLang = langPref.lang.split('-')[0];
break;
}
}
}
return userLang;
}

/*
* Looks up the language from the router renderProps.
* When that fails fall-back to accept-language.
*
*/
export function getFilteredUserLanguage({ renderProps, acceptLanguage } = {}) {
let userLang;
// Get the lang from the url param by default if it exists.
if (renderProps && renderProps.params && renderProps.params.lang) {
userLang = renderProps.params.lang;
}
// If we don't have a valid userLang set yet try accept-language.
if (!isValidLang(userLang) && acceptLanguage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the !isValidLang() bit would be clearer with the renderProps. I feel we should be able to trust renderProps is formatted correctly as well.

export function getFilteredUserLanguage({ renderProps, acceptLanguage } = {}) {
  if (isValidLang(renderProps.params.lang)) {
    return renderProps.params.lang;
  }
  return sanitizeLanguage(getLangFromHeader(acceptLanguage));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how we can trust renderProps is OK without checking it against the list of allowed values. It feels safer to assume we can't trust it to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean we should be able to trust that react-router gives us the same renderProps structure. So no need to do renderProps && renderProps.params && renderProps.params.lang. I think my code is missing a sanitizeLanguage() call in the if.

It feels like this function is doing a bit too much since it is finding possible langs, validating them and returning a match. I'm not sure if it's worth the refactor but it would be reasonable to me to make this essentially just do isValidLang(lang) ? sanitizeLang(lang) : defaultLang.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see now. One problem is that not all apps will have localization.

userLang = getLangFromHeader(acceptLanguage);
}
return defaultLang;
return sanitizeLanguage(userLang);
}
24 changes: 17 additions & 7 deletions src/core/server/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import ServerHtml from 'core/containers/ServerHtml';
import config from 'config';
import { setLang, setJWT } from 'core/actions';
import log from 'core/logger';
import { getDirection, getLangFromRouter, langToLocale } from 'core/i18n/utils';
import { getDirection, getFilteredUserLanguage, langToLocale } from 'core/i18n/utils';
import I18nProvider from 'core/i18n/Provider';
import Jed from 'jed';

Expand Down Expand Up @@ -83,6 +83,11 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) {
app.get('/', (req, res) => res.redirect(302, '/search'));
}

if (appInstanceName === 'disco' && isDevelopment) {
app.get('/', (req, res) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-router has a <Redirect /> component, can you add that to the router in disco/routes.js when in development instead?

res.redirect(302, '/en-US/firefox/discovery/pane/48.0/Darwin/normal'));
}

app.use((req, res) => {
if (isDevelopment) {
log.info('Running in Development Mode');
Expand Down Expand Up @@ -113,19 +118,24 @@ function baseServer(routes, createStore, { appInstanceName = appName } = {}) {
fs.readFileSync(path.join(config.get('basePath'), 'dist/sri.json'))
) : {};

const lang = getLangFromRouter(renderProps);
const acceptLanguage = req.headers['accept-language'];
// Get language from URL or fall-back to accept-language.
const lang = getFilteredUserLanguage({ renderProps, acceptLanguage });

if (renderProps.params && renderProps.params.lang) {
if (renderProps.params) {
const origLang = renderProps.params.lang;
if (lang !== origLang) {
// If the original lang param (found via the router) matches
// the first part of the original url path, redirect to the
// normalized lang (which will be the default if invalid).
// If a lang was provided but the lang looked up is different
// redirect to the looked-up lang (or default).
// eslint-disable-next-line no-unused-vars
const [_, firstPart, ...rest] = req.originalUrl.split('/');
if (origLang === decodeURIComponent(firstPart)) {
// The '' provides a leading /
return res.redirect(301, ['', lang, ...rest].join('/'));
return res.redirect(302, ['', lang, ...rest].join('/'));
} else if (!origLang && config.get('redirectLangPrefix')) {
// If there was no lang param. Redirect to the same URL with
// a lang prepended.
return res.redirect(302, `/${lang}${req.originalUrl}`);
}
}
}
Expand Down
9 changes: 4 additions & 5 deletions src/disco/routes.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import React from 'react';
import { IndexRoute, Route } from 'react-router';
import { Router, Route } from 'react-router';

import App from './containers/App';
import DiscoPane from './containers/DiscoPane';

export default (
<Route path="/" component={App}>
<IndexRoute component={DiscoPane} />
<Router component={App}>
<Route
path="/:lang/firefox/discovery/pane/:version/:platform/:compatibilityMode"
path="/(:lang/)firefox/discovery/pane/:version/:platform/:compatibilityMode"
component={DiscoPane}
/>
</Route>
</Router>
);
Loading