Javascript file loader

[gabrielrobert]

This PR add the possibility to load javascript files using this format:

export default {
  nested: {
    content: "EN"
  }
}

This format is used by libraries like nuxt-i18n/vue-i18n.

[Androz2091]

The idea is interesting, but JavaScript files don't always start with export default. Sometimes it's module.exports, when using NodeJS. We could add a check for that type of export too.

[gilmarsquinelato]

@gabrielrobert I liked the idea, but I agree with @Androz2091, since the common way across every environment is module.exports maybe we can standardise to be that.
Another problem is about javascript nature, it can have imports, pre-processing things, and string concatenation, that at the end will be lost once it's saved by i18n manager.
Based on that I don't know how nice it would be. Since the beginning, I had in mind to bring a js loader to the project, but with it, comes a lot of problems, that I don't know how good it could be, and the amount of opened issues it could result.
Because it's not like json or yaml, that you just put text, you don't have pre-processing on them, and like json, you can import it on js projects, and use them as normal js objects.

[gabrielrobert]

Many thanks for your reviews. I just updated my branch to bring support for CommonJS exports.

Regarding files containing javascript instructions, I don't think it is a good idea to get processing using things like the eval function because it opens the door for security issues. This PR will bring minimal support efforts, it's just plain JSON/JS object wrapped in module system. Therefore, fancier usage (string concat, imports, etc) is not supported.

[gilmarsquinelato]

What I meant was not to support these features, but since it will be evaluated, and when the file be written once hit save, the previous format will be dropped, and I don't know how happy the users will be with that thing.
My concern is about the users start to raise loads of issues because they lost their files structure :)
One another thing that just came to my mind, what about code comments? We will drop this, what's the impact of this in translation files?

[gabrielrobert]

i18n-manager will not open files containing more advanced javascript features (including comments). A user cannot overwrite a file by error because it won't get loaded at all.

This plugin use JSON.parse and this API will fail for any content that is not JSON-friendly.

[gilmarsquinelato]

Or the plugin could read the file by using require, so instead of trying to parse and using regex, just read using require and write in a JSON format. This makes it smarter, then we come back to the concern about previous code removal.

[gabrielrobert]

I tested this initially and moved from it because it opens the same problems as the eval function. Also, I don't think there is a proper way to serialize the result back to javascript while keeping the same format as the input file.

[gilmarsquinelato]

Yeah, you're right.
So, don't you think, since we have some possible problems with our user base, to just keep not supporting js files?
I'm telling this because if it's just for basic support in a JSON way, would be better to just change the file to JSON, since in any js environment JSON is a common thing to import with no effort. And it's already fully supported XD.

[gabrielrobert]

It's not that simple. Some i18n libraries do not work with JSON files, but with JS dictionaries. This PR will bring support for these kinds of files while ignoring any advanced JS features.

At the end of the day, it's your call! I use nuxt-i18n with .js files. As a target user, I'm fully aware that JS instructions are not the best practice in localization files, and I don't expect much support from an external tool.

[gilmarsquinelato]

Got it, but, are you sure that they don't support even if you manually provide them? like, importing the files and then provide the objects.

[gabrielrobert]

Sure, they do. But in my case, I use more advanced javascript features, and I do pre-process those files before editing them with i18n-manager. For instance:

export default {
     countries: require('../../random_path/countries'),
     content: "hehe"
}

Become

export default {
     content: "hehe"
}

Then I can edit them. i18n-manager stay super useful even when the format is partially supported.

[gilmarsquinelato]

Understood, I see now the importance of it.

I was thinking and I don't think we need to care about security things, since the tool only runs locally, so there's no risk :)

And using eval sounds more reliable, so we don't need to care about punctuation and any other thing. Resulting in less work that results in less maintenance :D

What you think about that?

[gabrielrobert]

Correct me if I'm wrong, but Electron applications can run with administrative power. Someone could create a reproduction repo for you with malicious code getting executed when you open the .js file. I don't think it's good. Interesting link -> https://www.digitalocean.com/community/tutorials/js-eval

[Androz2091]

Users should be careful what file they open and make sure they have installed the correct version of i18n-manager 🤷

[gabrielrobert]

Hehehe, having this in mind, why does antivirus software still exist? 😜
I don't think punctuation is a problem at all. JSON or JS users still need to comply with JSON.parse, or it won't work at all.

[gilmarsquinelato]

@gabrielrobert I don't think it's the case

Firstly, Electron has a packaging thing that we can't write into it easily.
Second, I think it will be really hard to poison the app and re-distribute it, and the hacker will need to repack it to distribute.
Lastly, the users should not download from other sources than Github releases :D

Remember, if the user wants to do shit, they will do, and it's not our fault XD

[gilmarsquinelato]

@gabrielrobert about punctuation, was more about having ' or ", ending with , or ending the export with ;, with eval we don't need to care about any of them.

[gabrielrobert]

@gilmarsquinelato JSON.parse do not care about them either. You can put ; or not at the end of the file, and the plugin will handle it. You can try this out, I put some file in /testData.

Lastly, the users should not download from other sources than Github releases :D

The problem is not the Electron package itself, but the translation file you load. Let's say I put malicious code in testData/javascript/en.js and I open an issue. You will open i18n-manager, try to open this file, and get hijacked. i18n-manager will eval malicious bits.

[gilmarsquinelato]

@gabrielrobert ah, got it, very nice point of view 😄

Ok, so then I'll test it and approve 😄

One thing related to the testData files, make sure they aren't the same, I mean, have different ways, like one have " in the keys, the others don't, one another file ends with ;, and so on, with that, we cover all possible cases of the basic support.

And be explicit on the README that the js support is basic, and add a link to the testData/javascript folder so the users will know what they can expect.