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

Proposal: locale option and meta.messages revision #9870

Closed
mysticatea opened this issue Jan 21, 2018 · 13 comments
Closed

Proposal: locale option and meta.messages revision #9870

mysticatea opened this issue Jan 21, 2018 · 13 comments
Assignees
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint needs bikeshedding Minor details about this change need to be discussed

Comments

@mysticatea
Copy link
Member

mysticatea commented Jan 21, 2018

The purpose is to support the localization of error messages. This proposal includes two things.

  1. Adding the way to specify a locale.
  2. Adding the way to give the locale to rules/formatters.

locale option

The option to specify a locale. The value is BCP 47 language tag that the official Intl feature is using.

If this is not specified, it's the value of new Intl.Collator().resolvedOptions().locale by default. This is system locale. I confirmed that it's available on Node.js 4.0.0 the minimum version we are supporting.

CLI

eslint src --locale ja-JP

CLIEngine

const engine = new CLIEngine({ locale: "ja-JP" })

Linter

const linter = new Linter()
linter.verify(code, config, { locale: "ja-JP" })

meta.messages revision

There is two idea. I prefer A currently. But both have pros/cons, so probably we can provide both ways.

A. nested objects

This adds a new form meta.messages[locale][messageId].

  • If meta.messages[locale] is not found and locale has hyphen(s), it retries after it removes rear of the last hyphen. (See also https://tools.ietf.org/html/rfc4647#section-3.4)
  • If meta.messages[locale] is not found then it uses meta.messages[messageId].

For detail, Linter try to resolve locale with the following steps:

function getMessage(messages, messageId, locale) {
    do {
        if (typeof messages[locale] === "object" && typeof messages[locale][messageId] === "string") {
            return messages[locale][messageId]
        }

        // Strip after the last `-` (e.g. strip country code).
        const hyphen = locale.lastIndexOf("-")
        locale = (hyphen === -1) ? "" : locale.slice(0, hyphen)
    } while (locale)

    // Default value.
    return messages[messageId]
}

For example:

module.exports = {
    meta: {
        // Or `messages: resolve("./messages.json")` to separate file.
        messages: {
            getter: "Getter is not present.",
            setter: "Setter is not present."
            ja: {
                getter: "Getter が必要です。",
                setter: "Setter が必要です。"
            }
        }
    }
}
  • Pros:
    • Easy to use without depending filesystem. Especially, not depending filesystem is important because we have online demo.
  • Cons:
    • Hard to handle plural.

B. functions

This allows that meta.messages is a function. The first argument is the current locale.

For example:

module.exports = {
    meta: {
        messages(locale) {
            if (locale === "ja" || locale.startsWith("ja-")) {
                return {
                    getter: "Getter が必要です。",
                    setter: "Setter が必要です。"
                }
            }
            return {
                getter: "Getter is not present.",
                setter: "Setter is not present."
            }
        }
    }
}
  • Pros:
    • People can use i18n libraries in the function.
  • Cons:
    • People have to resolve the messages for the locale manually.

Custom formatters revision

This adds the second argument options. It's an object.

  • options.locale (string) ... the current locale.
//my-awesome-formatter.js
module.exports = function (results, options) {
    // options.locale is the locale value.

    console.log(JSON.stringify(results, null, 2));
}

Policy in core rules

We have to make policy for PRs of languages core team can't read.

  1. Wait for upvotes from multiple people in their country.
  2. Or we confirm it with Google translation.
@mysticatea mysticatea added needs bikeshedding Minor details about this change need to be discussed feature This change adds a new feature to ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jan 21, 2018
@mysticatea mysticatea self-assigned this Jan 21, 2018
@platinumazure
Copy link
Member

Off the cuff, I just wanted to note that I slightly prefer just enhancing ESLint core to accept "some sort of message localization object" specified by users in configuration; then users could do their own translation in separate projects, and the ESLint core team wouldn't have to worry about translations at all.

For example: I think this could be a new type of object that plugins could export, and it could contain a function that takes a rule ID and messageId (and message data object) and return a string if it has a rendering/translation available, and if it returned null/undefined/whatever, ESLint would fall back to the core infrastructure. ESLint could even allow multiple plugins to provide a translation/rendering function and ESLint core would try all of them.

In this way, I think the problem becomes a lot simpler.

I definitely think we could create an ESLint core rule translation project in the organization, if we really wanted, which would allow users to translate all the core rule messages in one place. But I don't think ESLint core should choose a particular implementation for localizing messages.

@mysticatea
Copy link
Member Author

@platinumazure Is this what you intend?

// eslint-plugin-ja
module.exports = {
    getMessages(ruleId, messageId) {
        // return the message.
    }
}

And config:

// .eslintrc.json
{
    "plugins": ["ja"] // it will change rule's messages.
}

Indeed, this is nice to maintain a language in a separated repo.

On the other hand, it makes hurdle a bit for users, as needing some additional configuration. TypeScript 2.6 introduced locale option (https://blogs.msdn.microsoft.com/typescript/2017/10/31/announcing-typescript-2-6/), it's charming. 🤔

@ilyavolodin
Copy link
Member

I agree with @platinumazure that localization should be located in a separate package/file. My suggestion would be that instead of accepting local, we accept path to the file/package that contains localization. When that path is passed in, we modify context.report to look into that file first for appropriate messageId.

@not-an-aardvark
Copy link
Member

Is there a way to do this that doesn't require localizers to hardcode a rule ID? It seems like an external localizer might only explicitly support core rules/popular plugins, but it would be nice to get localized messages for rule forks (e.g. the ones in eslint-plugin-babel) or rules from eslint-rule-composer, which would have the same messages and messageIds. Otherwise, this might make it harder for people to switch to custom rules if a core rule doesn't meet their needs.

@ilyavolodin
Copy link
Member

We could modify context.report to only require rule name (not fully qualified package/rule name). That should allow it to work for both core rules and eslint-plugin-babel, however, it might result in errors, if somebody create a plugin rule that has the same name as core rule, but uses different messages.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jan 26, 2018

It seems like we would have a similar problem if a plugin upgrades to new version and its messages change.

To me, it seems like rule naming is a significant design problem because we've incorrectly assumed that a package/name string identifier is enough to globally identify a rule. In fact, this isn't the case because plugins update their rules occasionally, and users can load custom rules. This naming problem has effectively blocked us from having plugins as dependencies of shareable configs, because we can't support multiple rules with the same name.

As another example, it would be possible to implement localization entirely in userland using eslint-rule-composer by just creating a set of new rules that wrap the existing rules and transform the messages. Unfortunately, this would be very inconvenient to use because all the rule names would need to be different, so it wouldn't be compatible with any existing configs. If we solved that problem somehow (e.g. by allowing plugins to directly modify the rule namespace, or allowing users to refer to a different rule without modifying the names in their shareable configs), then localization might already be solved. (Of course, this would be a bigger proposal and require significantly more design.)

In the long term, I think our new features should move away from globally addressing rules with strings. Instead, I think we should add things to rule metadata so that we can perform operations on rules directly, and we can ensure that rules compose easily without needing to keep track of a name.

@mysticatea
Copy link
Member Author

I still want to consider in order to use user's language without additional configuration, such as typescript.

How about a step in the release script?

For example, I manage Japanese messages in eslint/locale-ja repository. And the release script takes localized messages from the repository before publishing.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 30, 2018

Sorry about the long delay in responding.

Personally, I'd be fine with bundling locales in the eslint npm package, although it seems like it would be simpler if everything that goes into the eslint npm package is also in the eslint/eslint GitHub repository. I'm not sure there would be an advantage to putting eslint/locale-ja in a separate repository; if it would get published as part of eslint anyway, we might as well just include it in the same repository.


My main concern about having i18n packages maintained separately from the plugins that they apply to is that i18n packages shouldn't have to use plugin/name rule ID strings to identify a particular rule. This would cause compatibility issues when upgrading a plugin, and it would often cause conflicts if we end up supporting having multiple loaded plugins with the same name in the future. The problem is fundamentally that an i18n package would be trying to apply messages to a specific plugin that it knows nothing about, aside from the name.

I think it's important that whatever solution we decide on here is compatible with a future scenario where shareable configs can have plugins as dependencies, e.g. as described in #10643. In this case, the author of a shareable config would be responsible for the version of a plugin that the shareable config uses, so the details of that plugin would be outside of the end user's control. It's not clear to me how the end user would be able to reliably configure i18n for a plugin like that. It seems like any i18n solution installed by the end user independently of plugins would restrict the shareable config author's ability to upgrade their plugin, because the messages in the new version of the plugin might be incompatible with the old messages.

Presumably, an i18n package would be targeted at a specific version of a specific plugin, so it would be better for the plugin to be a dependency or peerDependency of the i18n package so that the i18n author can ensure that their package being applied to the correct plugin. But making the plugin a dependency of the i18n package would effectively require a config that enables rules from the plugin to instead use the i18n package as a replacement. Making the plugin a peerDependency of the i18n package would require the i18n package to be installed by the same entity that installs the plugin (in the future, this might be a config author rather than an end user). In either case, this would require cooperation from a shareable config author.


To summarize, I'm having a hard time seeing a solution that meets all of the following criteria:

  1. i18n is configurable by end users without cooperation from shareable config authors.
  2. i18n is reliable; translations don't suddenly stop working if a new version of a transitive third-party dependency is published.
  3. Shareable config authors can upgrade plugins without cooperation from end users.
  4. i18n packages are developed and maintained independently from plugins.

My impression is that (1) and (2) are necessary for i18n to be useful (although I'm a native English speaker and I probably wouldn't be a user of i18n for most plugins, so please correct me if I'm wrong about these). I think (3) is very important for the future of shareable configs.

In your opinion, how important is (4)? It seems like adding i18n to plugins directly would be the simplest solution, and it would avoid all of the issues about how an i18n package knows which plugin it's operating on. It would also decouple i18n from the issue of how plugins are named and loaded. In cases where adding i18n to a third-party plugin isn't possible, an end user could still work around the issue by creating a "wrapper" plugin that copies the rules from the third-party plugin and adds i18n support (although this wouldn't affect versions of the plugin that are loaded by third-party shareable configs).

I'm not sure any solution here is ideal for everyone, but I think adding i18n to plugin packages directly might be the best way forward given the set of constraints.

@chrissinclairedited
Copy link

Just chiming in on this as I'd like to see this for a related use case #10705.

Firstly - I just wanted to say thank you to all of you for your work on this package, it's been invaluable in transforming JS generally into a language that can be used at scale. I think this feature is important for the community (even as an english-only speaker), and credit to you all for taking it seriously.

More on topic - I'd generally prefer for localisation to be handled outside the main eslint package. I don't see the need in bundling messages for multiple languages across many projects when most of the time they aren't needed (e.g. as the projects I'm working on use React, why would I want to include localised versions of Angular specific rules? Especially as I'd assume that most projects are only going to want one or two other languages than English). There's already a setup for plugins, so I'd suggest leveraging that. ESLint is included in many other OSS projects, and so bundling localisations within the package could lead to them being downloaded multiple times depending on how well npm / yarn de-dup the package tree. My gut feeling is the impact here would be on CI build times. I'd rather not pay the price for extended build times for features I'm not using.

I like @mysticatea's comment about zero config. I wonder if we could reduce this to just installing e.g. eslint-config-prettier-ja and passing a --locale ja flag to enable Japanese translations for prettier? Versioning would potentially be an issue, and it would be between (in that case) eslint-config-prettier and eslint-config-prettier-ja to manage. Having eslint-config-prettier-ja declare eslint-config-prettier as a peer dependency with a strict SemVer range should help (at least in theory). At least this way if a shared config were to update eslint-config-prettier to a version that wasn't supported by a locally installed eslint-config-prettier-ja, we'd see a warning about missing peer-dependencies.

I agree with @not-an-aardvark that changing rule ids would potentially be painful for localisation packages. The flip side to this is that to my mind the rule id is part of the public API for the package, and changing that should be a new major version. That should limit the chance of that kind of issue cropping up on transitive dependencies.

In terms of the implementation of the localised packages - could it not be as simple as a function that gets passed a single warning / error (complete with appropriate context, e.g. the data provided to context.report, line numbers etc.) and the current message and then leaves it as an implementation detail to that specific package exactly how it generates the translation. If it's happy to use a static lookup file, then it can, and equally if it needs to do something at runtime it can? It can then also decide how to handle the case where it doesn't know about a new rule (e.g. pass back the generated english message but add a warning about a missing translation?).

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 31, 2018

I'd generally prefer for localisation to be handled outside the main eslint package. I don't see the need in bundling messages for multiple languages across many projects when most of the time they aren't needed (e.g. as the projects I'm working on use React, why would I want to include localised versions of Angular specific rules?)

To clarify, I meant that localizations for rules would be bundled in the same place as the rules themselves. In other words, ESLint core would include localizations for the rules that are bundled with ESLint core, and eslint-plugin-react would include localizations for the rules that are bundled with eslint-plugin-react. So you wouldn't end up downloading localizations for Angular rules unless you were using a plugin that contained Angular rules.

@chrissinclairedited
Copy link

Ah apologies - that makes sense. Thanks!

aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jan 18, 2019
kaicataldo pushed a commit that referenced this issue Jan 18, 2019
* Chore: use meta.messages in global-require

* Chore: use meta.messages in guard-for-in

* Chore: use meta.messages in handle-callback-err

* Chore: use meta.messages in id-blacklist

* Chore: use meta.messages in id-length

* Chore: use meta.messages in id-match

* Chore: use meta.messages in implicit-linebreak

* Chore: use meta.messages in indent-legacy

* Chore: use meta.messages in indent

* Chore: use meta.messages in jsx-quotes

* Chore: use meta.messages in init-declarations

* Chore: use messageId in rule key-spacing

* Chore: use messageId in rule linebreak-style

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule max-len

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule line-comment-position

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule lines-around-comment

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule lines-around-directive

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule lines-between-class-members

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule max-depth

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* global-require

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule max-lines-per-functions

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule max-lines

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule max-nested-callbacks

* Chore: use messageId in rule max-params

* Chore: use messageId in rule max-statements-per-line

* Chore: use messageId in rule max-statements-per-function

* Chore: fix max-lines-per-function

* Chore: use messageId in rule max-statements

* Chore: use messageId in rule new-cap

* Chore: use messageId in rule multiline-ternary

* Chore: use messageId in rule multiline-comment-style

* Chore: use messageId in rule newline-after-var

* Chore: use messageId in rule new-parens

* Chore: use messageId in rule newline-before-return

* Chore: use messageId in rule newline-per-chained-call

* Chore: use messageId in rule no-async-promise-executor

* Chore: use messageId in rule keyword-spacing

* Chore: use messageId in rule yoda

* Chore: fix wrap-iife (+1 squashed commits)

Squashed commits:

[9959b8f] Chore: use messageId in rule wrap-iife

* Chore: use messageId in rule unicode-bom

* Chore: use messageId in rule use-isnan

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule valid-typeof

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule vars-on-top

Signed-off-by: weiran.zsd <weiran.zsd@outlook.com>

* Chore: use messageId in rule yield-star-spacing

* Chore: use messageId in rule valid-jsdoc

* Chore: use messageId in rule template-curly-spacing

* Chore: use messageId in rule template-tag-spacing

* Chore: use meta.messages in switch-colon-spacing

* Chore: use meta.messages in strict

* Chore: use meta.messages in symbol-description

* Chore: use meta.messages in no-duplicate-import

* Chore: use meta.messages in no-fallthrough

* Chore: use meta.messages in space-unary-ops

* Fix: rule spae-unary-ops messages

* Chore: use messageId in rule space-unary-ops

* Chore: use messages in rule no-floating-decimal

* Chore: use messages in rule no-invalid-meta

* Fix: rule space-unary-ops messages

* Chore: use messages in rule no-unexpected-multiline

* Chore: use messages in rule no-unsafe-negation

* Chore: use messages in rule no-undef

* Chore: use messages in rule no-unused-labels

* Chore: use messages in rule operator-assignment

* Chore: use messages in rule prefer-const

* Chore: fix failing tests

* Chore: fix cli-engine tests failing

Signed-off-by: weiran.zsd <weiran.zsd@alibaba-inc.com>

* Chore: convert no-unsafe-negation to single messagesId + data (+3 squashed commits)
Squashed commits:
[407f0f2] Fix: yield-star-spacing messages

Signed-off-by: weiran.zsd <weiran.zsd@alibaba-inc.com>
[66ab408] Chore: do not dynamically generating messageId
[ff4a3b12] Chore: indent rule messageId expected => wrongIndentation
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Mar 25, 2019
@mysticatea mysticatea added the core Relates to ESLint's core APIs and features label Mar 30, 2019
@kaicataldo
Copy link
Member

@mysticatea Would it make sense to put this proposal through the RFC process now that we have that?

@nzakas
Copy link
Member

nzakas commented Aug 14, 2023

Closing, as it's been four years with no progress.

@nzakas nzakas closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2023
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 11, 2024
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint needs bikeshedding Minor details about this change need to be discussed
Projects
Archived in project
Development

No branches or pull requests

7 participants