-
Notifications
You must be signed in to change notification settings - Fork 44
langParser.js: Generate language list dynamically #136
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
base: master
Are you sure you want to change the base?
Conversation
andrewda
left a comment
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? |
src/js/languages.json
Outdated
| @@ -0,0 +1 @@ | |||
| {"English":"en","Español":"es","Norsk bokmål":"nb_NO","język polski":"pl"} No newline at end of 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.
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 |
| ]), | ||
| 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.jsto 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.jsfile is ignored. Adds tests. Renamesnb_NO.jsontonb-NO.jsonto comply with RFCstandards.
Closes: #125