-
Notifications
You must be signed in to change notification settings - Fork 45
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
lib/langParser.js
Outdated
const fs = require('fs') | ||
const iso639 = require('iso-639-1') | ||
|
||
fs.readdir(`${__dirname}/../static/i18n`, function(err, items) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ES6 syntax: (err, items) => {
lib/langParser.js
Outdated
var avaliableLanguages = { | ||
Languages: {}, | ||
} | ||
for (var i = 0; i < items.length; i++) { |
There was a problem hiding this comment.
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
lib/langParser.js
Outdated
fs.readdir(`${__dirname}/../static/i18n`, function(err, items) { | ||
var avaliableLanguages = { | ||
Languages: {}, | ||
} |
There was a problem hiding this comment.
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 = {}
There was a problem hiding this comment.
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.
lib/langParser.js
Outdated
const iso639 = require('iso-639-1') | ||
|
||
fs.readdir(`${__dirname}/../static/i18n`, function(err, items) { | ||
var avaliableLanguages = { |
There was a problem hiding this comment.
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
.
lib/langParser.js
Outdated
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) |
There was a problem hiding this comment.
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
.
lib/langParser.js
Outdated
`${__dirname}/../src/js/languages.js`, | ||
'export default ' + | ||
JSON.stringify(avaliableLanguages) + | ||
' // eslint-disable-line' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
@andrewda it needs to be changed to nb-NO as per ISO and RFC standards compliance for the npm package |
We cant rename |
Ok. |
lib/langParser.js
Outdated
}) | ||
fs.writeFileSync( | ||
`${__dirname}/../src/js/languages.js`, | ||
'export default ' + |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try require()
?
There's a webpack plugin to generate json file Using that will prevent us from having another js script be added to build script. |
Ok. Should I write all logic within |
Not really, you can import/require |
@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"} |
There was a problem hiding this comment.
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"}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
8c56b9a
to
e2ebd2a
Compare
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
ack 6948c27 |
@@ -104,6 +106,7 @@ module.exports = { | |||
]), | |||
new ExtractTextPlugin(`[name]${hash}.css`), | |||
new ManifestPlugin(), | |||
new GenerateJsonPlugin('../src/js/languages.json', generatedLangList), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
This adds
langParser.js
to generate the availablelanguage 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. Renamesnb_NO.json
tonb-NO.json
to comply with RFCstandards.
Closes: #125