[FIX JENKINS-39225] Internationalisation for Blue Ocean and JDL #556

scherler · 2016-10-06T17:39:54Z

Description

See JENKINS-35845.

https://youtu.be/tV_PhY76uus

Submitter checklist

Link to JIRA ticket in description, if appropriate.
Change is code complete and matches issue description
Appropriate unit or acceptance tests or explanation to why this change has no tests
Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
Ran Acceptance Test Harness against PR changes.

Reviewer checklist

Run the changes and verified the change matches the issue description
Reviewed the code
Verified that the appropriate tests have been written or valid explanation given

@jenkinsci/code-reviewers @reviewbybees

scherler · 2016-10-06T17:47:45Z

related to this jenkinsci/jenkins#2400

kzantow · 2016-10-06T22:00:15Z

blueocean-web/src/main/js/i18n.js

+    // have a common namespace used around the full app
+    ns: ['common'],
+    defaultNS: 'common',
+    preload: ['en', 'de'],


Assume this would go away?

in the current form yeah

kzantow · 2016-10-06T22:00:47Z

blueocean-web/src/main/js/i18n.js

+      escapeValue: false // not needed for react!!
+    },
+    resources: {
+      de: {


And there's a way to load things per locale? (You mentioned this in our brief chat, so assume so, but we'll need to make sure it works that way)

yeah that is just me testing. We would have multiple locations the ones from extension points and the other as we chatted the existing jenkins ones. That will come now

kzantow · 2016-10-06T22:02:06Z

blueocean-web/src/main/js/main.jsx

@@ -37,17 +39,18 @@ class App extends Component {
    }

    render() {
+        const { t } = this.props;


big 👎 on props usage for this, need something globally... this is not a react specific thing

we are passing functionality to the component as you do in react. Why should we not use a plugin such as react-i18next for the underlying i18next provider.

http://i18next.com/docs/api/

In case you need it outside the react way see above api, but in React components the above is the way to go IMHO

kzantow · 2016-10-06T22:02:53Z

blueocean-web/src/main/js/main.jsx

-                            <Link to="/pipelines">Pipelines</Link>
-                            <a href="#">Administration</a>
+                            <Link to="/pipelines">{t('pipelines')}</Link>
+                            <a href="#">{t('administration')}</a>


good syntax, though. i like this better than most of the other i18n providers I've found, which add react components and makes them a non-starter for me

I like the brevity of this syntax too, although I think we should just get a reference to "t" via import / require.

kzantow · 2016-10-06T22:03:46Z

blueocean-web/src/main/js/main.jsx

-        <Provider store={store}>
-            <Router history={history}>{ makeRoutes(routes) }</Router>
-        </Provider>
+          <I18nextProvider i18n={ i18n }>


why does this have anything to do with react? we should not use react specific stuff here, just make the t localization function available globally

that is the react enabler see above comment for non-react components. They would use i18n service directly.

You asked about blueocean-web/src/main/js/i18n.js that is a simple configure factory for the i18n service this can be used in a non-react context.

kzantow

I assume you have some experience with this library, so I have not problem with it so far, seems as good as anything I've seen (and yes, I did look at this one before, too). Only thing I am adamant about is not making this react specific in any way but rather providing a localization function more-or-less globally to the app.

tfennelly · 2016-10-07T08:05:32Z

@scherler Hey Thor ... how are the localized language bundles defined with this? How are they loaded etc?

tfennelly · 2016-10-07T08:11:01Z

blueocean-web/src/main/js/i18n.js

+          logout: 'Logout',
+          pipelines: "Pipelines",
+          administration: "Administration",
+        }


Ok ... now I think I see how the resource bundles are defined. So if I have a plugin, how do I contribute localised strings?

Right, ideally it shouldn't matter if they are strings in classic UI, server side messages, or client side js/react - same .properties right?

tfennelly · 2016-10-07T08:16:41Z

Hey Thor ... I do think it would be better to make this a normal Javascript component that can be imported (required). In general ... I don't see the point in making something a react component (or any other framework component) unless it absolutely needs to be.

tfennelly · 2016-10-07T08:26:50Z

So to be a bit more specific Thor .... what seems to make sense to me is a simple API that you can call that looks kinda similar to what I think you have here e.g.

import LanguageBundle from i18n;

// Get a pack for the "editorcreate" string set.
const editorCreateI8n = new LanguageBundle('editorcreate');

/// blah blah blah

render () {
    return {
        <div>{editorCreateI8n('start')}</div>
    };
}

scherler · 2016-10-07T11:33:41Z

@tfennelly @kzantow This is a really early snapshot and far from being usable. WIP!!! The integration with extension points and how we will define the languages is still to be determined. I opened the PR so you guys have a preview.

I decided for http://i18next.com/ since it is a pure javascript library which support big language bundles as you both request above. Regarding the integration of that library into our projects (I feel tended to remind you we are using react) I have chosen https://github.com/i18next/react-i18next.

However blueocean-web/src/main/js/i18n.js is that "LanguageBundle" you are talking about which can be imported from any POJSO. Anyway that is as well still in heavy development.

tfennelly · 2016-10-07T11:42:18Z

@scherler yeah I have not forgotten that we are using react as it keeps me awake at night sometimes 😄 . The fact that we are using react does not mean that we have to "reactify" absolutely everything. There's actually a name for that faulty logic / antipattern that I'm sure you're very familiar with .... "Golden Hammer".

So ... I'm all for reactifying this and other things if there's a real tangible benefit to it and those benefits out-weight the downsides. I'm not seeing any such benefits here yet. Sorry bud !!

scherler · 2016-10-07T12:02:47Z

@tfennelly The benefit to use a framework like i18next is that it supports big files but delivers them with optimisations. Features like missingKey for translations can be saved to the resources, white listing languages, namespaces, etc.

In react we inject the t function with translate(['common'], { wait: true })(App) and then use it with this.props.t('key') however you can do the same in a non react way http://i18next.com/translate/

tfennelly · 2016-10-07T12:23:58Z

@scherler Thor ... I think you are missing my point. I'm totally fine with using the i18next package. What I do not agree with (atm anyway) is the reactification of that via react-i18next. It strikes me as a classic example of over/mis application of a technology (i.e. Golden Hammer).

So it sounds to me like we can use i18next directly in react and non-react code. That sounds perfect to me. I would not introduce multiple ways of using this stuff and I would not introduce more than is needed.

I've said this before but I'll just throw it out here again .... I think we need to be really careful how we use react (and libs in general). Among other things ... we need to minimize the exposure we have to them so that we are not hammered with backward compatibility issues down the line when things change. So, I really am against using every react feature we can uncover, just for the sake of using it, especially when there's a perfectly fine "standard" way of doing things.

and yes Thor .... that is a c&p of what I said on CrapChat 😃 ... I hate retyping.

kzantow · 2016-10-07T14:30:25Z

@scherler @tfennelly agree 100% with Tom on this... same thing I was saying, let's ditch any react specific stuff for now and just make this available as a global library - lots of benefits for doing this, but mostly it makes things consistent and you can always still reference in react dom and properties easily. i18next package is fine, but with a wrapper, probably (which might negate using it at all). I'd like to see a slightly different syntax than Tom, maybe. My ideal thing is more like:

var t = require('translations')('my-plugin-name'); // getting a reference to the right thing could be different... ask me next week if there's a much easier way (there is, maybe in the works)

// later, as an example in react, in this case:
<table>
...
<th>{t.userName}</th>
<th>{t.userList.birthday}</th>
<th>{t.userList.age}</th>
<th>{t.userList.worth}</th>
<th>{t.userList.successMessageTitle}</th>
...
<td>{user.name}</td>
<td>{t.shortDate(user.birthday)}</td>
<td>{t.number(user.age)}</td>
<td style={{background: t.userList.dollarAmountBackgroundImage}}>{t.currency(user.bankRoll)}</td>
<td>{t.userList.successMessage(user.successPhrase)}</td>
...

This should be super easy in javascript to accomplish, check this:

function blah(arg) {
    return 'blah ' + arg;
}
blah.toString = function() {
    return 'blah';
}

console.log('blah', blah)
console.log('blah(hello)', blah('hello'))

Then just make it dependent if there are parameters to replace, and do a recursive lookup for further message resolutions, and you have about the best system you can/need.

Sorry, I'm quite opinionated about this because I've written systems for this in Java before, which worked great. And since this is something fundamental and used by everything, we need to make it as easy as possible to do.

tfennelly · 2016-10-07T15:55:27Z

@kzantow hmmm ... I see what you're doing but it seems a bit funky for my liking for a v1. I'd personally prefer to see it start more simple + use something others have already built Vs reinventing the wheel (wrapping it is fine).

Anyway ... my main concern is that we do not reactify-the-f*ck out of everything 😄 . As for the smaller details of the syntax ... I'm sure we could do it many diff ways and we'd all have slightly different preferences, so I'm happy enough with whatever @scherler comes up with.

kzantow · 2016-10-07T16:02:46Z

@tfennelly here's the problem: we have been prone to getting things wrong the first time. So ok, even if my ideas are wrong, at least they're extremely easy to use. The issue is when we get things wrong that are very verbose, we have more things to change. We have more code that's been written to adapt to these verbose solutions and we have bunches of code to change to adapt to the newly discovered 'right' way of doing things. Let's please stop doing that and get things closer the first time instead of iterating over and over. We're implementing a framework at this point. Frameworks are supposed to be easy to use. Let's be thought leaders, not peons implementing stuff because it needs to be done according to some agile timeframe.

cliffmeyers

I am inclined to agree w/ @kzantow and @tfennelly: it would be better if we have a single consistent API for I18N that is used in React components and POJOs. Would also mean one less React-specific library to depend on which IMO is a good thing since managing React upgrades is going to be tricky enough long-term.

Does this library do date / time formatting? That to me is an important requirement, or at least understanding we'll need to roll our own if not.

tfennelly · 2016-10-07T16:14:18Z

@kzantow Getting it wrong the first time is not a bad thing and is to be expected. We've gotten things wrong the second time too. Thinking we're going to get things right the first time is not realistic. It's exactly because I'm not expecting us to get it right first time (or maybe even second) that I'm suggesting we should not go ott on it or go reinventing the wheel. Keep it simple if we can is all I'm suggesting + reuse the work and knowledge gained by others - 100K downloads of i18next per month ... it's probably not perfect, but must be doing something right and thinking we're going to pull something better out of our cheeks with min effort is crazy talk imo (and would be very arrogant of us).

kzantow · 2016-10-07T18:07:31Z

@tfennelly maybe, but is it arrogant to want something better? Are you saying that it's arrogant to know something is better from experience? I understand your rationale for keeping things simple, but it seems that's been biting us left and right for months now...

If I'm hearing you correctly, you are literally saying: let's not do it the best way we know how, because some other people do it some other way. I fundamentally don't agree with that. Even if we decide later that we were wrong, at least we were striving for it. The thing is... I have implemented this before, albeit in Java in a short time. It's an easy task to get right.

But the thing I can't stress enough: we're building a Jenkins plugin framework for the front end. We're not building some random npm module that people will use on their hipster website because they're too lazy to implement their own... like leftpad. We have problems to solve that hardly anyone else does and providing half-hearted solutions to them only serves to dissuade people from integrating with it.

Last thing: look at how this works in Jenkins today. If it's not as easy, we need a better solution. Jenkins is dirt simple to localize. Would you would advocate doing something worse?

tfennelly · 2016-10-08T07:39:57Z

@kzantow you must be trolling me, are you? Go on ... admit it 😉

I might regret bitting, but 😄 ....

Imo, we collectively as a team "Now" jack sh1t on this in the sense that we do not have agreement. To "know" something as an individual is, to me, no different than just having a strong opinion, which is nice, but still just an opinion. We have to get some sort of agreement, which is hard.

The only thing that can possibly be up for debate here in this case when making the argument for reinventing the wheel is the proposed usage model/syntax. I admit I've only looked at i18next and what they have in their docs Vs the samples shown here. Seriously, nobody is going to tell me that we "know" a proposed syntax that we have zero agreement on is, or ever will be, so radically "better" than their working syntax that it warrants spending X amount of time reinventing the wheel. After that it's a non-contest as far as I'm concerned ... working code with tests, docs, a community of people contributing to it for the last number of years (5 listed on the top level org ... 78 on the core module), 100K downloads a month Vs no working code, tests or anything else.

As for experience ... I and others here have a little bit of that too. I've lost count of how many times I've seen people fall into the trap of "we know better than everyone else", before heading off to waste time solving problems that others have already solved very well. As I said, I have not looked at any other of these OSS libs for doing i18n, but from my sample of 1 that I have seen, I can already see that we need a much stronger case for reinventing the wheel. Personal taste arguments about the positioning of a dot (or whatever) is not good enough imo.

Everything out there done by other people is not sh1t just coz it's not done the way I might have done it. It really infuriates me when people dismiss other peoples work so easily. For me, it's kinda like listening to a climate change or religion Vs science debate. On one side you have the people that have done all the hard work and put it out there (warts and all) for others to pick holes in. On the other side you have ...

sophistifunk · 2016-11-04T04:42:09Z

I've been trying to just let this float by rather than jump in and make it even noisier, but are you guys all sure you're talking about the same thing, and that thing is actually XSS?

If we're fetching message raw resources via REST they should be returned un-processed with the substitutions happening client-side. If we're fetching localised messages, then they should only have values mapped into the templates from server-side data. The only thing coming from the client should be entity ids.

michaelneale · 2016-11-04T04:55:26Z

@scherler are you concerned if that some unsafe HTML method is used, then the values from elsewhere (not the Messages.properties) may create some danger? (unsafe html injection is always a worry, you are right to be concerned) If so, being conservative is good, but I don't think markdown is the right way, certainly not to start with.

I think from what cliff is saying, we shouldn't have markup in the translations (as often it is very implementation specific), but I am curious if this is something that is occuring elsewhere in jenkins.

I say to move on, we just use text only in the content of translations until we know for sure we need something else.

* [JENKINS-35845] rework i18n keys for home page * [JENKINS-35845] l10n for activity tab * [JENKINS-35845] l10n for branches tab * [JENKINS-35845] l10n for pull requests tab * [JENKINS-35845] l10n for run details -> pipeline * [JENKINS-35845] l10n for run details -> changes, tests, artifacts * [JENKINS-35845] l10n for run details header changes * [JENKINS-35845] pagination; fix typo causing error

… to default values. Fix tests of all related projects.

ghost · 2016-11-10T19:35:34Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

michaelneale · 2016-11-14T01:02:57Z

@scherler I see this is marked for review, but has some failing tests still (you can see on CI link above).

scherler · 2016-11-15T10:38:08Z

@michaelneale yeah, the problem with windows not defined :( I just tried to fix that but now personalization tests are not working at all. :(

michaelneale · 2016-11-16T04:28:51Z

@scherler ok, thanks.

scherler added 4 commits October 5, 2016 18:43

[JENKINS-35845] WIP first steps with i18n

785aaaa

eslint - formating changes and fix offences

426aae9

Merge remote-tracking branch 'origin/master' into JENKINS-35845

e2c57da

[JENKINS-35845] first basic working version with 2 languages

ea448c7

scherler added the work-in-progress label Oct 6, 2016

kzantow reviewed Oct 6, 2016

View reviewed changes

tfennelly reviewed Oct 7, 2016

View reviewed changes

scherler added the DO NOT MERGE label Oct 7, 2016

scherler self-assigned this Oct 7, 2016

kzantow changed the title ~~FIX Jenkins-35845 Internationalisation for Blue Ocean and JDL~~ [FIX JENKINS-35845] Internationalisation for Blue Ocean and JDL Oct 7, 2016

cliffmeyers reviewed Oct 7, 2016

View reviewed changes

scherler and others added 5 commits November 7, 2016 12:19

[JENKINS-35845] remove debug statement

32a18f1

Merge remote-tracking branch 'origin/master' into JENKINS-35845

6f5d441

[JENKINS-35845] fix import

49a9929

[JENKINS-35845] order keys

54fd4bf

oleg-nenashev mentioned this pull request Nov 9, 2016

[FIXED JENKINS-39604] - ResourceBundleUtil#getBundle() should report misses on the low level jenkinsci/jenkins#2627

Merged

scherler added 6 commits November 9, 2016 17:12

[JENKINS-35845] WIP security commit to be able to merge master

2a3e8d5

Merge remote-tracking branch 'origin/master' into JENKINS-35845

4901735

eslint - formating changes and fix offences

714043f

[JENKINS-35845] Remove dep to snapshot-jenkins and implement fallback…

8ba0a73

… to default values. Fix tests of all related projects.

[JENKINS-35845] remove duplicate variable

2b06258

[JENKINS-35845] fix test by adding polyfy again

9fb6222

scherler added needs-review and removed DO NOT MERGE work-in-progress labels Nov 10, 2016

eslint - formating changes and fix offences

62f91ba

michaelneale approved these changes Nov 13, 2016

View reviewed changes

[JENKINS-35845] Fix last test

51b86c6

scherler added 3 commits November 17, 2016 17:51

[JENKINS-35845] fix tests for dashboard

c243acd

Merge remote-tracking branch 'origin/master' into JENKINS-35845

d398a38

[JENKINS-35845] sync version numbers

4a1ea80

scherler merged commit ed76c2c into master Nov 17, 2016

scherler deleted the JENKINS-35845 branch November 17, 2016 18:15

svijee mentioned this pull request Dec 10, 2016

[JENKINS-39225] Improved (reverted) German translation of Branch(es) … #654

Merged

8 tasks

[FIX JENKINS-39225] Internationalisation for Blue Ocean and JDL #556

[FIX JENKINS-39225] Internationalisation for Blue Ocean and JDL #556

Conversation

scherler commented Oct 6, 2016 • edited Loading

Description

Submitter checklist

Reviewer checklist

scherler commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzantow left a comment

Choose a reason for hiding this comment

tfennelly commented Oct 7, 2016

tfennelly Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfennelly commented Oct 7, 2016

tfennelly commented Oct 7, 2016 • edited Loading

scherler commented Oct 7, 2016

tfennelly commented Oct 7, 2016

scherler commented Oct 7, 2016

tfennelly commented Oct 7, 2016 • edited Loading

kzantow commented Oct 7, 2016 • edited Loading

tfennelly commented Oct 7, 2016

kzantow commented Oct 7, 2016

cliffmeyers left a comment

Choose a reason for hiding this comment

tfennelly commented Oct 7, 2016

kzantow commented Oct 7, 2016 • edited Loading

tfennelly commented Oct 8, 2016 • edited Loading

sophistifunk commented Nov 4, 2016

michaelneale commented Nov 4, 2016 • edited Loading

ghost commented Nov 10, 2016

michaelneale commented Nov 14, 2016

scherler commented Nov 15, 2016

michaelneale commented Nov 16, 2016

scherler commented Oct 6, 2016 •

edited

Loading

tfennelly Oct 7, 2016 •

edited

Loading

tfennelly commented Oct 7, 2016 •

edited

Loading

tfennelly commented Oct 7, 2016 •

edited

Loading

kzantow commented Oct 7, 2016 •

edited

Loading

kzantow commented Oct 7, 2016 •

edited

Loading

tfennelly commented Oct 8, 2016 •

edited

Loading

michaelneale commented Nov 4, 2016 •

edited

Loading