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

langParser.js: Generate language list dynamically #136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mooncos
Copy link
Member

@mooncos mooncos commented Jan 14, 2018

This adds langParser.js to generate the available
language list automatically from available translation
files. This also adds a npm script for generating before
webpack bundles. Modifies .gitignore so that generated
languages.js file is ignored. Adds tests. Renames
nb_NO.json to nb-NO.json to comply with RFC
standards.

Closes: #125

Copy link
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

Also, the rename from nb_NO to nb-NO seems unrelated to this PR.

const fs = require('fs')
const iso639 = require('iso-639-1')

fs.readdir(`${__dirname}/../static/i18n`, function(err, items) {
Copy link
Member

Choose a reason for hiding this comment

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

ES6 syntax: (err, items) => {

var avaliableLanguages = {
Languages: {},
}
for (var i = 0; i < items.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

items.forEach(...) is significantly cleaner in this case

fs.readdir(`${__dirname}/../static/i18n`, function(err, items) {
var avaliableLanguages = {
Languages: {},
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to have a nested object? Would be much cleaner to just use availableLanguages = {}

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to be so that we can import all nested objects at once.

const iso639 = require('iso-639-1')

fs.readdir(`${__dirname}/../static/i18n`, function(err, items) {
var avaliableLanguages = {
Copy link
Member

Choose a reason for hiding this comment

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

Typo - should be availableLanguages.

Also, use ES6's const instead of var.

for (var i = 0; i < items.length; i++) {
var langName = items[i].substring(0, items[i].indexOf('.'))
var normalizedName = langName.split('-')[0]
var nativeName = iso639.getNativeName(normalizedName)
Copy link
Member

Choose a reason for hiding this comment

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

All of these can be const instead of var.

`${__dirname}/../src/js/languages.js`,
'export default ' +
JSON.stringify(avaliableLanguages) +
' // eslint-disable-line'
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty sketch. Can we write directly to a languages.json JSON file instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried with no success...

Copy link
Member Author

@mooncos mooncos Jan 14, 2018

Choose a reason for hiding this comment

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

Is it possible to import a JSON file in ES6? I can't seem to achieve it.

@mooncos
Copy link
Member Author

mooncos commented Jan 14, 2018

@andrewda it needs to be changed to nb-NO as per ISO and RFC standards compliance for the npm package

@jayvdb
Copy link
Member

jayvdb commented Jan 15, 2018

We cant rename static/i18n/nb_NO.jsonstatic/i18n/nb-NO.json . These filenames are provided by Weblate, so we must use them otherwise they think the translation is missing.

@mooncos
Copy link
Member Author

mooncos commented Jan 15, 2018

Ok.

})
fs.writeFileSync(
`${__dirname}/../src/js/languages.js`,
'export default ' +
Copy link
Member

Choose a reason for hiding this comment

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

A .json file saves the trouble of having to export things.

Copy link
Member Author

@mooncos mooncos Jan 15, 2018

Choose a reason for hiding this comment

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

And how can I import a JSON file?

Copy link
Member

Choose a reason for hiding this comment

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

import name from 'file.json'

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve tried without success

Copy link
Member

Choose a reason for hiding this comment

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

try require() ?

@blazeu
Copy link
Member

blazeu commented Jan 15, 2018

There's a webpack plugin to generate json file
https://www.npmjs.com/package/generate-json-webpack-plugin

Using that will prevent us from having another js script be added to build script.

@mooncos
Copy link
Member Author

mooncos commented Jan 15, 2018

Ok. Should I write all logic within webpack.config.js?

@blazeu
Copy link
Member

blazeu commented Jan 15, 2018

Not really, you can import/require langParser.js in webpack.config.js

@mooncos
Copy link
Member Author

mooncos commented Jan 15, 2018

@blazeu fixed that. but where is the generated JSON located?

@@ -0,0 +1 @@
{"English":"en","Español":"es","Norsk bokmål":"nb_NO","język polski":"pl"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • No newline at EOF.

Origin: SpaceConsistencyBear, Section: all.whitespace.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpeolv_bln/src/js/languages.json
+++ b/tmp/tmpeolv_bln/src/js/languages.json
@@ -1 +1 @@
-{"English":"en","Español":"es","Norsk bokmål":"nb_NO","język polski":"pl"}+{"English":"en","Español":"es","Norsk bokmål":"nb_NO","język polski":"pl"}

Copy link
Member

Choose a reason for hiding this comment

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

Can you make this pretty printed.

Also I guess it should be in static/ , but I am not 100% sure about that.

Copy link
Member

@andrewda andrewda Jan 16, 2018

Choose a reason for hiding this comment

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

Or .gitignore it because it's generated each time.

Copy link
Member Author

@mooncos mooncos Jan 16, 2018

Choose a reason for hiding this comment

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

I fixed that but I now have another problem: webpack reads the import statements before the languages.json file is generated. I don’t know how to fix this.

Copy link
Member

Choose a reason for hiding this comment

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

elliottsj/generate-json-webpack-plugin#5

Yup it is not possible to import the generated json, might have to use the previous method or search for another webpack plugin.

Copy link
Member

Choose a reason for hiding this comment

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

webpack reads the import statements before the languages.json file is generated.

When new language is added, the compiled js will still import the file that has the old language list. That is undesirable.

@mooncos mooncos force-pushed the auto-locales branch 2 times, most recently from 8c56b9a to e2ebd2a Compare January 15, 2018 17:42
This adds ``langParser.js`` to generate the available
language list automatically from available translation
files. This also adds a npm script for generating before
webpack bundles. Modifies .gitignore so that generated
``languages.js`` file is ignored. Adds tests.

Closes: coala#125
@jayvdb
Copy link
Member

jayvdb commented Jan 27, 2018

ack 6948c27

@jayvdb
Copy link
Member

jayvdb commented Jan 27, 2018

needs a rebase, and a recheck by @blazeu , or @wisn

@@ -104,6 +106,7 @@ module.exports = {
]),
new ExtractTextPlugin(`[name]${hash}.css`),
new ManifestPlugin(),
new GenerateJsonPlugin('../src/js/languages.json', generatedLangList),
Copy link
Member

Choose a reason for hiding this comment

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

In case you want to prettify the output.

GenerateJsonPlugin(<file>, <json>, [replacer], [space])

Fill out that space argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

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

Successfully merging this pull request may close these issues.

None yet

6 participants