From 9ce407966f7b8d11131420994cfc5d7476e3a985 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Tue, 4 May 2021 16:49:35 +0100 Subject: [PATCH] Improved theme locale handling - when activating a theme, we need to load the current locale - this request used to be buried deep in the themeI18n init call - now we surface it in the bridge and pass it down, which is closer to what we want to do with eventually initialising the frontend with everything it needs up front (or not initialising it, if it isn't needed) - in the related helpers we depend on the site.locale value instead of proxy -> themeI18n -> settingsCache drastically simplifying the code and removing deep requires - site.locale is updated via middleware and can be relied upon --- core/bridge.js | 12 ++++-- core/frontend/helpers/date.js | 6 ++- core/frontend/helpers/lang.js | 7 ++-- core/frontend/services/theme-engine/active.js | 22 ++++++++--- .../services/theme-engine/i18n/i18n.js | 17 ++------- test/unit/helpers/date_spec.js | 37 +++++++------------ test/unit/helpers/lang_spec.js | 36 ++++++++++-------- test/unit/helpers/t_spec.js | 13 ++----- .../unit/services/theme-engine/active_spec.js | 9 ++++- 9 files changed, 82 insertions(+), 77 deletions(-) diff --git a/core/bridge.js b/core/bridge.js index 2030332d4b..7907326281 100644 --- a/core/bridge.js +++ b/core/bridge.js @@ -14,15 +14,16 @@ const logging = require('./shared/logging'); const events = require('./server/lib/common/events'); const i18n = require('./shared/i18n'); const themeEngine = require('./frontend/services/theme-engine'); +const settingsCache = require('./server/services/settings/cache'); class Bridge { constructor() { /** * When locale changes, we reload theme translations - * @deprecated: the term "lang" was deprecated in favour of "locale" publicly 4.0 + * @deprecated: the term "lang" was deprecated in favour of "locale" publicly in 4.0 */ - events.on('settings.lang.edited', () => { - this.getActiveTheme().initI18n(); + events.on('settings.lang.edited', (model) => { + this.getActiveTheme().initI18n({locale: model.get('value')}); }); } @@ -31,6 +32,9 @@ class Bridge { } activateTheme(loadedTheme, checkedTheme, error) { + let settings = { + locale: settingsCache.get('lang') + }; // no need to check the score, activation should be used in combination with validate.check // Use the two theme objects to set the current active theme try { @@ -40,7 +44,7 @@ class Bridge { previousGhostAPI = this.getActiveTheme().engine('ghost-api'); } - themeEngine.setActive(loadedTheme, checkedTheme, error); + themeEngine.setActive(settings, loadedTheme, checkedTheme, error); const currentGhostAPI = this.getActiveTheme().engine('ghost-api'); if (previousGhostAPI !== undefined && (previousGhostAPI !== currentGhostAPI)) { diff --git a/core/frontend/helpers/date.js b/core/frontend/helpers/date.js index fbe5308972..672a7e802d 100644 --- a/core/frontend/helpers/date.js +++ b/core/frontend/helpers/date.js @@ -3,7 +3,7 @@ // // Formats a date using moment-timezone.js. Formats published_at by default but will also take a date as a parameter -const {SafeString, themeI18n} = require('../services/proxy'); +const {SafeString} = require('../services/proxy'); const moment = require('moment-timezone'); const _ = require('lodash'); @@ -26,6 +26,8 @@ module.exports = function (...attrs) { date = date === null ? undefined : date; const timezone = options.data.site.timezone; + const locale = options.data.site.locale; + const { format = 'll', timeago @@ -44,7 +46,7 @@ module.exports = function (...attrs) { // i18n: Making dates, including month names, translatable to any language. // Documentation: http://momentjs.com/docs/#/i18n/ // Locales: https://github.com/moment/moment/tree/develop/locale - dateMoment.locale(themeI18n.locale()); + dateMoment.locale(locale); if (timeago) { date = dateMoment.tz(timezone).from(timeNow); diff --git a/core/frontend/helpers/lang.js b/core/frontend/helpers/lang.js index 976e8d910f..a334adc42e 100644 --- a/core/frontend/helpers/lang.js +++ b/core/frontend/helpers/lang.js @@ -12,8 +12,9 @@ // Language tags in HTML and XML // https://www.w3.org/International/articles/language-tags/ -const {SafeString, themeI18n} = require('../services/proxy'); +const {SafeString} = require('../services/proxy'); -module.exports = function lang() { - return new SafeString(themeI18n.locale()); +module.exports = function lang(options) { + const locale = options.data.site.locale; + return new SafeString(locale); }; diff --git a/core/frontend/services/theme-engine/active.js b/core/frontend/services/theme-engine/active.js index 034bcd5bf9..83c675823e 100644 --- a/core/frontend/services/theme-engine/active.js +++ b/core/frontend/services/theme-engine/active.js @@ -30,13 +30,16 @@ class ActiveTheme { * @param {object} checkedTheme - the result of gscan.format for the theme we're activating * @param {object} error - bootstrap validates the active theme, we would like to remember this error */ - constructor(loadedTheme, checkedTheme, error) { + constructor(settings, loadedTheme, checkedTheme, error) { // Assign some data, mark it all as pseudo-private this._name = loadedTheme.name; this._path = loadedTheme.path; this._mounted = false; this._error = error; + // We get passed in a locale + this._locale = settings.locale || 'en'; + // @TODO: get gscan to return validated, useful package.json fields for us! this._packageInfo = loadedTheme['package.json']; this._partials = checkedTheme.partials; @@ -96,8 +99,17 @@ class ActiveTheme { return this._engines[key]; } - initI18n() { - themeI18n.init(this._name); + /** + * + * @param {object} options + * @param {string} [options.activeTheme] + * @param {string} [options.locale] + */ + initI18n(options = {}) { + options.activeTheme = options.activeTheme || this._name; + options.locale = options.locale || this._locale; + + themeI18n.init(options); } mount(siteApp) { @@ -129,8 +141,8 @@ module.exports = { * @param {object} checkedTheme - the result of gscan.format for the theme we're activating * @return {ActiveTheme} */ - set(loadedTheme, checkedTheme, error) { - currentActiveTheme = new ActiveTheme(loadedTheme, checkedTheme, error); + set(settings, loadedTheme, checkedTheme, error) { + currentActiveTheme = new ActiveTheme(settings, loadedTheme, checkedTheme, error); return currentActiveTheme; } }; diff --git a/core/frontend/services/theme-engine/i18n/i18n.js b/core/frontend/services/theme-engine/i18n/i18n.js index a0dda3f61b..589e5d0574 100644 --- a/core/frontend/services/theme-engine/i18n/i18n.js +++ b/core/frontend/services/theme-engine/i18n/i18n.js @@ -1,7 +1,6 @@ const errors = require('@tryghost/errors'); const i18n = require('../../../../shared/i18n'); const logging = require('../../../../shared/logging'); -const settingsCache = require('../../../../server/services/settings/cache'); const config = require('../../../../shared/config'); const isNil = require('lodash/isNil'); @@ -19,18 +18,18 @@ class ThemeI18n extends i18n.I18n { * * @param {String} activeTheme - name of the currently loaded theme */ - init(activeTheme) { + init({activeTheme, locale}) { // This function is called during theme initialization, and when switching language or theme. - const currentLocale = this._loadLocale(); + this._locale = locale || this._locale; // Reading file for current locale and active theme and keeping its content in memory if (activeTheme) { // Reading translation file for theme .hbs templates. // Compatibility with both old themes and i18n-capable themes. // Preventing missing files. - this._strings = this._tryGetLocale(activeTheme, currentLocale); + this._strings = this._tryGetLocale(activeTheme, this._locale); - if (!this._strings && currentLocale !== this.defaultLocale()) { + if (!this._strings && this._locale !== this.defaultLocale()) { logging.warn(`Falling back to locales/${this.defaultLocale()}.json.`); this._strings = this._tryGetLocale(activeTheme, this.defaultLocale()); } @@ -68,14 +67,6 @@ class ThemeI18n extends i18n.I18n { } } } - - /** - * Load the current locale out of the settings cache - */ - _loadLocale() { - this._locale = settingsCache.get('lang'); - return this._locale; - } } module.exports = ThemeI18n; diff --git a/test/unit/helpers/date_spec.js b/test/unit/helpers/date_spec.js index dabb30d9e2..b391ce38ac 100644 --- a/test/unit/helpers/date_spec.js +++ b/test/unit/helpers/date_spec.js @@ -4,17 +4,9 @@ const should = require('should'); // Stuff we are testing const helpers = require('../../../core/frontend/helpers'); -const proxy = require('../../../core/frontend/services/proxy'); -const settingsCache = require('../../../core/server/services/settings/cache'); - const moment = require('moment-timezone'); describe('{{date}} helper', function () { - afterEach(function () { - settingsCache.reset(); - proxy.themeI18n._loadLocale(); - }); - it('creates properly formatted date strings', function () { const testDates = [ '2013-12-31T11:28:58.593+02:00', @@ -74,36 +66,35 @@ describe('{{date}} helper', function () { const timezone = 'Europe/Dublin'; const format = 'll'; - const context = { - hash: {}, - data: { - site: { - timezone - } - } - }; - - locales.forEach(function (l) { - settingsCache.set('lang', {value: l}); - proxy.themeI18n._loadLocale(); + locales.forEach(function (locale) { let rendered; + const context = { + hash: {}, + data: { + site: { + timezone, + locale + } + } + }; + testDates.forEach(function (d) { rendered = helpers.date.call({published_at: d}, context); should.exist(rendered); - String(rendered).should.equal(moment(d).tz(timezone).locale(l).format(format)); + String(rendered).should.equal(moment(d).tz(timezone).locale(locale).format(format)); rendered = helpers.date.call({}, d, context); should.exist(rendered); - String(rendered).should.equal(moment(d).tz(timezone).locale(l).format(format)); + String(rendered).should.equal(moment(d).tz(timezone).locale(locale).format(format)); }); // No date falls back to now rendered = helpers.date.call({}, context); should.exist(rendered); - String(rendered).should.equal(moment().tz(timezone).locale(l).format(format)); + String(rendered).should.equal(moment().tz(timezone).locale(locale).format(format)); }); }); diff --git a/test/unit/helpers/lang_spec.js b/test/unit/helpers/lang_spec.js index a3e090565b..b804875e16 100644 --- a/test/unit/helpers/lang_spec.js +++ b/test/unit/helpers/lang_spec.js @@ -1,24 +1,28 @@ const should = require('should'); -const settingsCache = require('../../../core/server/services/settings/cache'); const helpers = require('../../../core/frontend/helpers'); -const proxy = require('../../../core/frontend/services/proxy'); describe('{{lang}} helper', function () { - beforeEach(function () { - settingsCache.set('lang', {value: 'en'}); - proxy.themeI18n._loadLocale(); - }); - - afterEach(function () { - settingsCache.shutdown(); - proxy.themeI18n._loadLocale(); - }); - it('returns correct language tag', function () { - let expected = proxy.themeI18n.locale(); - let rendered = helpers.lang.call(); + const locales = [ + 'en', + 'en-gb', + 'de' + ]; - should.exist(rendered); - rendered.string.should.equal(expected); + locales.forEach((locale) => { + const context = { + hash: {}, + data: { + site: { + locale + } + } + }; + + let rendered = helpers.lang.call({}, context); + + should.exist(rendered); + rendered.string.should.equal(locale); + }); }); }); diff --git a/test/unit/helpers/t_spec.js b/test/unit/helpers/t_spec.js index 29f01bd454..a2fb4ac306 100644 --- a/test/unit/helpers/t_spec.js +++ b/test/unit/helpers/t_spec.js @@ -12,12 +12,10 @@ describe('{{t}} helper', function () { afterEach(function () { configUtils.restore(); - settingsCache.shutdown(); }); it('theme translation is DE', function () { - settingsCache.set('lang', {value: 'de'}); - themeI18n.init('casper'); + themeI18n.init({activeTheme: 'casper', locale: 'de'}); let rendered = helpers.t.call({}, 'Top left Button', { hash: {} @@ -27,8 +25,7 @@ describe('{{t}} helper', function () { }); it('theme translation is EN', function () { - settingsCache.set('lang', {value: 'en'}); - themeI18n.init('casper'); + themeI18n.init({activeTheme: 'casper', locale: 'en'}); let rendered = helpers.t.call({}, 'Top left Button', { hash: {} @@ -38,8 +35,7 @@ describe('{{t}} helper', function () { }); it('[fallback] no theme translation file found for FR', function () { - settingsCache.set('lang', {value: 'fr'}); - themeI18n.init('casper'); + themeI18n.init({activeTheme: 'casper', locale: 'fr'}); let rendered = helpers.t.call({}, 'Top left Button', { hash: {} @@ -49,8 +45,7 @@ describe('{{t}} helper', function () { }); it('[fallback] no theme files at all, use key as translation', function () { - settingsCache.set('lang', {value: 'de'}); - themeI18n.init('casper-1.4'); + themeI18n.init({activeTheme: 'casper-1.4', locale: 'de'}); let rendered = helpers.t.call({}, 'Top left Button', { hash: {} diff --git a/test/unit/services/theme-engine/active_spec.js b/test/unit/services/theme-engine/active_spec.js index 07729d96f4..490b68132c 100644 --- a/test/unit/services/theme-engine/active_spec.js +++ b/test/unit/services/theme-engine/active_spec.js @@ -15,6 +15,7 @@ describe('Themes', function () { describe('Mount', function () { let engineStub; let configStub; + let fakeSettings; let fakeBlogApp; let fakeLoadedTheme; let fakeCheckedTheme; @@ -23,6 +24,10 @@ describe('Themes', function () { engineStub = sinon.stub(engine, 'configure'); configStub = sinon.stub(config, 'set'); + fakeSettings = { + locale: 'en' + }; + fakeBlogApp = { cache: ['stuff'], set: sinon.stub(), @@ -45,7 +50,7 @@ describe('Themes', function () { // setup partials fakeCheckedTheme.partials = ['loop', 'navigation']; - const theme = activeTheme.set(fakeLoadedTheme, fakeCheckedTheme); + const theme = activeTheme.set(fakeSettings, fakeLoadedTheme, fakeCheckedTheme); // Check the theme is not yet mounted activeTheme.get().mounted.should.be.false(); @@ -76,7 +81,7 @@ describe('Themes', function () { // setup partials fakeCheckedTheme.partials = []; - const theme = activeTheme.set(fakeLoadedTheme, fakeCheckedTheme); + const theme = activeTheme.set(fakeSettings, fakeLoadedTheme, fakeCheckedTheme); // Check the theme is not yet mounted activeTheme.get().mounted.should.be.false();