New: Add context.report({ messageId }) (fixes #6740)

[j-f1]

What is the purpose of this pull request? (put an "X" next to item)

Add something to the core

Fixes #6740.

• Read the pull request guide (http://eslint.org/docs/developer-guide/contributing/pull-requests)
• Include tests for this change
• Update documentation for this change (if appropriate)

What changes did you make? (Give an overview)
I added a new messageId parameter to context.report:

context.report({
  messageId: 'foo'
})

This takes the value foo key from meta.messages:

module.exports = {
  meta: {
    // ...
    messages: {
      foo: "Your message here"
    }
  },
  create(context) {
    // ...
  }
}

You can also use them in RuleTester tests:

ruleTester.run('rule-name', rule, {
  valid: [
    // ...
  ],
  invalid: [
    {
      code: 'Something here',
      errors: [
        {
          messageId: 'foo'
          // if you have {{ }} markers in your message, add a `data` key here:
          // data: {
          //   myKey: 'my value'
          // }
        }
      ]
    }
  ]
})

---

If my example (accessor-pairs) looks good, I’ll be happy to convert some more rules.

[mention-bot]

@j-f1, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @roadhump and @platinumazure to be potential reviewers.

[platinumazure]

Nice! This looks pretty good at first glance, I'll give it a proper review later. Thanks for taking the time to write this- I'm looking forward to this getting in.

[eslintbot]

LGTM

[not-an-aardvark]

Thanks for the pull request! I left some suggestions.

[platinumazure]

@not-an-aardvark Regarding internationalization, don't you think it might make more sense for rules to own the localization process? At any rate, I'm not sure formatters should be responsible for it... At least, that was what I had thought originally.

[not-an-aardvark]

@platinumazure I was thinking maybe formatters would be a better solution because they would solve the issue of "how to configure i18n" (it would be configured by simply selecting a formatter for the user's language). While the current API is a step in the right direction, as far as I can tell it doesn't appear to allow any configuration of message mappings, which would be necessary for i18n. Also, delegating i18n to formatters might help take the burden off of the authors of popular plugins, so they wouldn't have to maintain many different translations in addition to the rule code itself.

Having said that, I don't have a perfect solution to the issue, given that passing messages to formatters before doing interpolation might require a significant API change. So I'm not opposed to this implementation -- I just want to make sure we've considered the details if we're planning to use this as a way to do i18n.

[j-f1]

This is now mergeable again @not-an-aardvark. Can you take another look at it?

I’m also wondering whether the API should be messageId or messageID. What do you think?

[not-an-aardvark]

Well done fixing the merge conflicts, I know a lot of the underlying code has changed in the past week 😄

I left a few more comments. The main thing I want to address is the design question of where we're expecting i18n to happen.

I’m also wondering whether the API should be messageId or messageID. What do you think?

I think it should be messageId, for consistency with the ruleId property on reported problems from the API.

[not-an-aardvark]

Can you avoid mutating the descriptor here? I think it would be better to put the messageId logic in interpolate so that all of the message-handling logic is in the same place, and the message doesn't need to be reassigned.

(Unlike #9165 (comment), this time I'm not concerned about mutating objects from rules, since I see you cloned the descriptor above. I just think not mutating a function's arguments in general is a good idea for code cleanliness, since it makes code easier to follow.)

[j-f1]

Hm, this isn’t actually a function argument:

// lib/report-translator.js:262
const descriptor = normalizeMultiArgReportCall.apply(null, arguments);

[not-an-aardvark]

Oops, you're right. I still think it's slightly better to avoid mutation in general, but I don't feel strongly about it here, so there's no need to change it.

[not-an-aardvark]

I'm confused -- do we want to include the messageId in the problem, or not? (Also cc'ing @platinumazure, since this is more of a design question)

Sorry if it seems like I'm going back and forth on this. This change adds some complexity to the rule API, and it will be difficult to make modifications to the API later without breaking compatibility. So I would feel better about the change knowing that (a) it provides benefits that clearly outweigh the additional complexity costs, and (b) we've thoroughly discussed the design details to ensure that those benefits will actually be accessible with the API that we expose.

I'm assuming you added problem.messageId in response to my suggestion about integrations in #9165 (comment). Thanks for doing that, although I'm unsure whether it's useful for an integration to have access to the messageId if they don't also have access to the data that was used as part of message interpolation, since that data would be needed to reconstruct the message or use i18n.

I'm a bit unclear about what the design goal of this feature is (maybe @platinumazure can help explain). From what I can tell (feel free to correct me if I describe anything incorrectly), one of the following is the case:

1. We want i18n to be handled by plugins and rule authors. In that case, it seems like message IDs are an internal detail and don't need to be exposed as part of the final problem object. (I'm unsure whether messageIds provide a significant a benefit over manual string concatenation in this case, although there might be something I'm missing.)
2. We want i18n to be handled by integrations. In that case, we would probably need to let the integrations handle message interpolation as well, otherwise they won't have enough info to translate messages.

I'm not trying to stand in the way here -- I just want to make sure we're communicating the design goals clearly, so that the resulting API is coherent and useful.

[j-f1]

How does this sound?

1. Remove the messageId from the report
2. Open an issue to allow overriding the message-generating infrastructure
3. Allow the values in the messages object to be functions that get called to retrieve the value. Usage:

   module.exports = {
     meta: {
       messages: {
         foo: (optionalDataParam) => myI18nLib('message here')
       }
     }
   }

[not-an-aardvark]

That seems like it could work, although I'd also like to hear what @platinumazure thinks.

[j-f1]

@platinumazure Any comments on this design?

[kaicataldo]

@platinumazure Friendly ping

[platinumazure]

Ack, I didn't see this thread when I commented earlier today. Something like the proposed design could also work, but I'm a fan of some sort of core-level hook that would allow i18n/l10n to be separate from rules, if desired.

[j-f1]

I’d also appreciate feedback on the IDs I’ve used in the rules I upgraded.

[eslintbot]

LGTM

[j-f1]

I’m not sure why the AppVeyor build failed. Closing and reopening to restart the build.

[j-f1]

Still failing. Any ideas why @not-an-aardvark?

[not-an-aardvark]

It seems to be unrelated to this PR -- the last two builds on master have failed too (commits).

[not-an-aardvark]

@j-f1 I think the issue is fixed now (see #9307)

[j-f1]

@not-an-aardvark Any thoughts on this?

[not-an-aardvark]

My thoughts are pretty much the same as they were in #9165 (comment).

Sorry about the delay here -- I realize it's frustrating for a PR to not get merged for awhile, and I'm hoping we can resolve this soon. That said, whatever we decide to do here will effectively need to be supported indefinitely, so I think it's worth taking the time to get the details right.

[platinumazure]

I'm really sorry, work has nearly killed me but I'm not dead yet. Hoping to address questions this week... I greatly appreciate everyone's patience and I want to thank @j-f1 for sticking with this. Really appreciate it.

[eslintbot]

LGTM

[platinumazure]

Work continued to twist the knife over the last few weeks... Really sorry.

I was thinking about the i18n dilemma. Here's my rough thinking on it:

1. Actually designing a solid i18n mechanism that would work for everyone is probably beyond the scope of this change 😄
2. Rules/plugins themselves are a pretty good place to maintain i18n and l10n, if the author desires. For plugin authors, this solution would allow them to immediately start internationalizing and localizing their messages if desired. (This would be done using gettext or similar right in the plugin.) That said, see last point below.
3. Integrations/formatters are probably not a good place to maintain l10n, because a formatter could not be expected to know about plugins unrelated to it.
4. An ideal solution would allow for the possibility of injecting l10n from somewhere else, as a separate entity that is neither an integration/formatter nor the rule/plugin itself.

Here's an example of what I have in mind for the last (but again, probably beyond the scope of this PR):

• ESLint core would create a message internationalization hook which would allow users to specify a messageLocalizers array (name open for bikeshedding) in configuration. Each of the messageLocalizers would receive the rule name and messageId, and would be responsible for looking up an internationalized version of the message or just returning the original message.
  • Best case would be if the message localizer could just return the message template and ESLint could hydrate it with the data from the original problem report, to ensure separation of concerns.
  • For plugins that are happy to supply their own i18n, maybe they could also export a messageLocalizer that could be set in config?
• Each of the messageLocalizers would be tried in order, and the first one that can localize the message would include a property indicating this success in its response.
• ESLint core would also need to support one (or some) ways of getting the user's preferred language code. A configuration option or CLI option could help here. In addition, ESLint could choose to support any commonly known/used environment variables for this purpose.

I think with a design like that, it would be possible to allow for one or more full internationalization/localization libraries to develop that supports localizing rules in whatever languages they choose to support, for whatever set of rules/plugins they choose to support.

Personally, I think it would be okay to avoid trying to solve the i18n problem in this PR.

@not-an-aardvark @j-f1 Any thoughts on this?

[j-f1]

@not-an-aardvark and @platinumazure Can you take another look when you have a chance? I think I’ve addressed all of the issues you’ve brought up (except for i18n support in formatters, but that’s a whole different can of worms).

[ilyavolodin]

@not-an-aardvark @platinumazure Could you review this again please? It's been sitting open for a very long time, and looks pretty good to me.

I'm not sure when I'll have time to rereview, but I don't want to block this, so feel free to merge it if everyone else is fine with it.

[j-f1]

@platinumazure Friendly ping?

[platinumazure]

Hi @j-f1, I do apologize. I still need to chat with @not-an-aardvark about the semver/API compatibility implications he raised. This PR will make message changes backward-incompatible and we need to figure out if that's something we can safely allow, and/or if there's anything we can do to avoid this.

I'm sorry I haven't had a chance to figure this out sooner.

[j-f1]

Proposal from above:

How does this sound?

1. Remove the messageId from the report
2. Open an issue to allow overriding the message-generating infrastructure
3. Allow the values in the messages object to be functions that get called to retrieve the value. Usage:

   module.exports = {
     meta: {
       messages: {
         foo: (optionalDataParam) => myI18nLib('message here')
       }
     }
   }

[platinumazure]

@j-f1 How would a message overriding infrastructure look at a high level? If there is no messageId, then would message overriding tools be passed the un-hydrated message or the hydrated message? How would this address the semver concerns brought up by @not-an-aardvark?

Edit: My mistake, I see that messageId is still used, it's just not in the report object. So we would have some sort of infrastructure that would allow a user to override (for example) rule=no-unused-vars, messageId=foo, and provide either a function that takes a data parameter, or a string formatted the way we format our messages today. I'm not sure this really solves the problem of having the rule message parameters effectively become part of the public contract, but that problem would only need to be addressed in the later proposal for message overrides.

Okay, that's good enough for me. Do any further changes need to be made to this PR to pivot in the suggested direction, or do you think this is ready to merge?

@not-an-aardvark I'd appreciate your thoughts on whether the proposal will solve your semver concerns.

[j-f1]

Do any further changes need to be made to this PR to pivot in the suggested direction, or do you think this is ready to merge?

None that I can think of, so this should be good to go.

[platinumazure]

LGTM, thanks so much for your patience as we figured everything out.

[kaicataldo]

LGTM. Thanks for contributing to ESLint!