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

Custom locale loader #35

Closed
wants to merge 7 commits into from

Conversation

stevelacey
Copy link
Contributor

@stevelacey stevelacey commented Jan 12, 2020

Adds the ability to swap out loadLocaleMessages for your own implementation.

{
    settings: {
        'vue-i18n': {
            localeDir: 'resources/lang/**/*.json',
            localeLoader(pattern) {
                const _ = require('lodash')
                const glob = require('glob')
                const messages = {}

                glob.sync(pattern).forEach(file => {
                    _.set(messages, file.match(/resources\/lang\/(.*)\.json/)[1].replace(/\//g, '.'), require(`./${file}`))
                })

                return [{ messages: messages.en }]
            }
        }
    }
}

This particular loader I've written allows me to load a structure like this, where the sub directories contain json that is flattened, the tree is implied by the path.

resources
└── lang
    ├── en
    │   ├── events.json
    │   ├── pages
    │   │   ├── app.json
    │   │   ├── assets.json
    │   │   ├── stats.json
    │   │   └── team.json
    │   ├── validation.json
    ├── en.json
    ├── ko
    │   └── validation.json
    └── ko.json

I'll add tests if you're happy with the implementation / naming / return values - let me know if there are any tweaks I should make.

A future work that would be great is to wrap in multiple locale check support. I'm not sure how to even approach it – but it would certainly related to the loader function in the end.

I should note that this also addresses the issue I created last year #32 – by supplying the ability to use a custom loader function users could load js based locales, or any format they want.

@stevelacey
Copy link
Contributor Author

stevelacey commented Jan 12, 2020

I've refined this further to instead accept locale and messages as per Vue i18n, and centralize the settings validation which now needs to check for more keys.

This allows me to use these settings:

    settings: {
        'vue-i18n': {
            locale: 'en',
            messages: (() => {
                const _ = require('lodash')
                const glob = require('glob')
                const messages = {}

                glob.sync('./resources/lang/**/*.json').forEach(file => {
                    _.set(messages, file.match(/resources\/lang\/(.*)\.json/)[1].replace(/\//g, '.'), require(file))
                })

                return messages // {en: {...}, ko: {...}}
            })()
        },
    },

The messages key could also accept a function. I made it an object for parity with vue-i18n.

This still lacks multiple locale support, but this seems closer.

What do you think?

Copy link
Member

@kazupon kazupon left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!
great! 👍

the ability to load it programmatically by user is great,
I think it would be nice to have some directory pattern settings, such as i18n-ally.
https://github.com/antfu/i18n-ally#-directory-structure
/cc @antfu

function getLocaleMessages (context) {
const settings = getSettings(context)

if (settings.locale && settings.messages) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need settings.locale ?
We need to cover everything all locales resources.

In about configuration name of settings.message, I think this feature is for loading resources with complicated structure. For this reason, the name should be intuitive, such as loadLocaleMessages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this design was because I misunderstood how the library currently supports multiple locales, I've provided more details below.

@stevelacey
Copy link
Contributor Author

stevelacey commented Jan 13, 2020

@kazupon I only just appreciated how your library supports multiple locales – in that it'll currently assume that every json file must define every key (because its assumed there'll be one per locale).

I'll update my design.

I went with loadLocaleMessages originally, but simplified the language because I figure this could well end up being the primary way consumers load locales.

I can't speak for general usage but I certainly adapted my vue-i18n usage to support eslint-plugin-vue-i18n (I switched to json; as well as making other compromises).

I figured terse language might make sense here, breaking up i18n into multiple files probably won't be a poweruser activity once it's supported, I'd expect many want to do it, translations get unwieldy fast otherwise.

@stevelacey
Copy link
Contributor Author

stevelacey commented Jan 13, 2020

@kazupon okay how about with these amends, the syntax for settings would now be like this, similar to the loadLocaleMessages return value:

    settings: {
        'vue-i18n': {
            locales: (() => {
                const _ = require('lodash')
                const glob = require('glob')
                const locales = {}

                glob.sync('./resources/lang/**/*.json').forEach(file => {
                    _.set(locales, file.match(/resources\/lang\/(.*)\.json/)[1].replace(/\//g, '.'), require(file))
                })

                return _.toPairs(locales).map(locale => {
                    return { name: locale[0], messages: locale[1] }
                })
            })()
        },
    },

AKA simplified:

    settings: {
        'vue-i18n': {
            locales: [
                { name: 'en', messages: {...} },
                { name: 'fr', messages: {...} },
            ],
        },
    },

The name attribute doesn't do anything but it could be used in error output. I haven't done anything with it because I note that your existing implementation doesn't /appear/ to do anything similar with the file/path attributes; but I would assume the idea there was that test output would describe which locale has missing translations. I don't see that anywhere currently. Am I missing it?

@stevelacey
Copy link
Contributor Author

@kazupon ping

@kazupon
Copy link
Member

kazupon commented Feb 10, 2020

sorry, I'm busy to work and to support Vue 3. 🙇
Could you wait until those are finishing, please ?

@egyel
Copy link

egyel commented Apr 25, 2020

Looking forward to use this enhancement.

@stevelacey
Copy link
Contributor Author

@kazupon any update? shall I rebase this?

@ota-meshi
Copy link
Member

ota-meshi commented Nov 8, 2021

Hi @stevelacey. Can you still continue to work on this PR?
If you can work on this PR, I will review this PR. However, I became the maintainer after this PR was opened, so I need to catch up on discussions about this PR.

@ota-meshi
Copy link
Member

I close this PR because it is not active.
I'm sorry we couldn't proceed until the merge. Thank you very much for your progress so far. If you come back, we welcome your PR.

@ota-meshi ota-meshi closed this Nov 16, 2021
@stevelacey
Copy link
Contributor Author

stevelacey commented Nov 16, 2021

Yeah I'd love to get this in but it needs entirely rewriting now given you've switched to TypeScript, I'm still using my forked branch on a couple of projects and it works great.

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

Successfully merging this pull request may close these issues.

4 participants