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

fix: removal of timeago fallback middleware #7259

Merged
merged 6 commits into from Jan 17, 2019
Merged

Conversation

@julianlam
Copy link
Member

julianlam commented Jan 16, 2019

Instead of loading English fallback on missing language, we opt
to not send a script tag for a missing language to begin with.

Timeago already loads with English as default, so it will just
continue to use English.

Instead of loading English fallback on missing language, we opt
to not send a script tag for a missing language to begin with.

Timeago already loads with English as default, so it will just
continue to use English.
@julianlam julianlam added the bug label Jan 16, 2019
@julianlam julianlam added this to the 1.12.0 milestone Jan 16, 2019
@julianlam julianlam self-assigned this Jan 16, 2019
@julianlam julianlam requested review from pitaj and barisusakli Jan 16, 2019
}

const valid = languages.map(obj => obj.code);
if (!valid.includes(res.locals.config.userLang)) {

This comment has been minimized.

Copy link
@pitaj

pitaj Jan 16, 2019

Contributor

languages.some(x => (userLang === x.code))

}

const pathToLocaleFile = '/vendor/jquery/timeago/locales/jquery.timeago.' + utils.userLangToTimeagoCode(res.locals.config.userLang) + '.js';
fs.access(path.join(__dirname, '../../public', pathToLocaleFile), function (err) {

This comment has been minimized.

Copy link
@pitaj

pitaj Jan 16, 2019

Contributor

file.exists

This comment has been minimized.

Copy link
@pitaj

pitaj Jan 16, 2019

Contributor

As I understand it, this is here because the ms language code exists in NodeBB but not in timeago, so the file doesn't exist. Perhaps we could just special case that instead.

@julianlam

This comment has been minimized.

Copy link
Member Author

julianlam commented Jan 16, 2019

ms is the testing case, it is a NodeBB language code but is not defined in timeago, so it should just not inject the footer script to load ms timeago translations.

src/middleware/header.js Outdated Show resolved Hide resolved
src/middleware/header.js Show resolved Hide resolved
src/middleware/header.js Outdated Show resolved Hide resolved
@julianlam julianlam merged commit c831ff0 into master Jan 17, 2019
5 checks passed
5 checks passed
ci/dockercloud Your tests passed in Docker Cloud
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 89.714%
Details
license/cla All CLA requirements met.
julianlam added a commit that referenced this pull request Jan 18, 2019
* fix: removal of timeago fallback middleware

Instead of loading English fallback on missing language, we opt
to not send a script tag for a missing language to begin with.

Timeago already loads with English as default, so it will just
continue to use English.

* fix: check userLang against supported language codes

* fix: cleaned up code as per @pitaj

* fix: added comments

* fix: more fixes as per @pitaj

* feat: added addl. test for timeago locales, fixed broken test
@julianlam julianlam modified the milestones: 1.12.0, 1.11.2 Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.